-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow DNS pod selectors to be configured #987
Allow DNS pod selectors to be configured #987
Conversation
cmd/sonobuoy/app/args.go
Outdated
func AddDNSPodLabelsFlag(str *[]string, flags *pflag.FlagSet) { | ||
flags.StringArrayVar( | ||
str, "dns-pod-labels", config.DefaultDNSPodLabels, | ||
"The label selectors to use for locating DNS pods during preflight checks.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we clarify in the help how to specify multiple ones? Its a comma-delimited list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I'm so glad you asked me about this. It encouraged me to do a bit more testing locally and realised I was using the wrong pflag function and that it didn't do what I thought it did!
"github.com/vmware-tanzu/sonobuoy/pkg/client" | ||
"github.com/vmware-tanzu/sonobuoy/pkg/config" | ||
"github.com/vmware-tanzu/sonobuoy/pkg/errlog" | ||
imagepkg "github.com/vmware-tanzu/sonobuoy/pkg/image" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it was already a problem before this PR but yours is touching it now; ideally the vmware-tanzu/sonobuoy imports would be in their own block (2nd block) and the external ones like cobra/pflag would be in a 3rd block (with stdlib being the first block)
pkg/client/preflight.go
Outdated
"github.com/pkg/errors" | ||
"github.com/sirupsen/logrus" | ||
"github.com/vmware-tanzu/sonobuoy/pkg/buildinfo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same comment about fixing these blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but a few nits if you want to fix them.
The preflight checks for Sonobuoy assume the namespace and labels of the DNS pods. Although the preflight checks can be skipped, some users may want to still run these checks and ensure that they pass before testing. To allow this, this change introduces two new flags that are applied to the `PreflightConfig` so that the namespace and labels to use for finding the DNS pods can be configured. The two new flags are: `--dns-namespace` and `--dns-pod-labels`. The default values are the values that were previously being used so this does not impact existing users. Signed-off-by: Bridget McErlean <[email protected]>
fd52819
to
a8d39f1
Compare
Codecov Report
@@ Coverage Diff @@
## master #987 +/- ##
==========================================
- Coverage 47.69% 47.62% -0.07%
==========================================
Files 76 76
Lines 5298 5314 +16
==========================================
+ Hits 2527 2531 +4
- Misses 2617 2627 +10
- Partials 154 156 +2
Continue to review full report at Codecov.
|
What this PR does / why we need it:
The preflight checks for Sonobuoy assume the namespace and labels of the
DNS pods. Although the preflight checks can be skipped, some users may
want to still run these checks and ensure that they pass before testing.
To allow this, this change introduces two new flags that are applied to
the
PreflightConfig
so that the namespace and labels to use forfinding the DNS pods can be configured.
The two new flags are:
--dns-namespace
and--dns-pod-labels
.The default values are the values that were previously being used so
this does not impact existing users.
Signed-off-by: Bridget McErlean [email protected]
Which issue(s) this PR fixes
Special notes for your reviewer:
I've verified that this change works with an OpenShift 4.x cluster by creating one locally using CRC.
Release note: