Skip to content
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

Add ability to override pod spec options #837

Merged

Conversation

@zubron
Copy link
Member

zubron commented Aug 14, 2019

What this PR does / why we need it:
This change introduces the ability to override PodSpec options for
plugins. A new flag --show-default-podspec has been added to the
gen and gen plugin commands. When used, Sonobuoy will include the
default PodSpec options for the given plugin driver in the resulting
yaml. This allows users to change these values or they can provide their
own. If one is provided in the plugin definition, Sonobuoy will use that
as is, otherwise it will use the default.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

Which issue(s) this PR fixes

Special notes for your reviewer:

Release note:

Sonobuoy now includes the option to edit the PodSpec used when running plugins. Users can provide their own PodSpec or adapt the default one used by Sonobuoy. The default can be obtained using the '--show-default-podspec' flag with the 'gen' and 'gen plugin' commands.
@@ -78,6 +78,9 @@ type GenConfig struct {
// out of band from the plugins because of how the dynamic plugins are not
// yet able to be manipulated in this way.
PluginEnvOverrides map[string]map[string]string

// ShowDefaultPodSpec

This comment has been minimized.

Copy link
@johnSchnake

johnSchnake Aug 14, 2019

Contributor

todo: add a godoc style comment that adds info, not just avoids the warning :-p


// DefaultPodSpec returns the default pod spec used for the given plugin driver type.
func DefaultPodSpec(d string) (v1.PodSpec, error) {
switch d {

This comment has been minimized.

Copy link
@johnSchnake

johnSchnake Aug 14, 2019

Contributor

I believe I tried to avoid caring too much about the casing; on the server when dealing with plugins I just strings.ToLower the driver to ensure I don't fail someone for daemonset vs Daemonset

@zubron zubron force-pushed the zubron:customise-pod-options-add-pod-spec-809 branch from e16cb3a to 7abbd78 Aug 14, 2019
case "Job":
return defaultJobPodSpec(), nil
default:
return v1.PodSpec{}, errors.Errorf("unknown driver %q", d)

This comment has been minimized.

Copy link
@johnSchnake

johnSchnake Aug 14, 2019

Contributor

You could also avoid dealing with the error case here at all and just do something like default to the defaultJobPodSpec when it is unknown. I think that is a pretty reasonable thing to do instead of blowing up at this stage. Just makes it a sane default.

That would just simplify the signature and call sites of this. Just an idea; not a requirement.

This comment has been minimized.

Copy link
@zubron

zubron Aug 14, 2019

Author Member

👍 That's a sensible default. Adding the error caused a chain of changes elsewhere that I'd rather avoid.

@zubron zubron force-pushed the zubron:customise-pod-options-add-pod-spec-809 branch 6 times, most recently from de5ef64 to 766f1a9 Aug 14, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 14, 2019

Codecov Report

Merging #837 into master will decrease coverage by 0.46%.
The diff coverage is 53.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #837      +/-   ##
==========================================
- Coverage   46.65%   46.19%   -0.47%     
==========================================
  Files          75       75              
  Lines        4730     4788      +58     
==========================================
+ Hits         2207     2212       +5     
- Misses       2393     2446      +53     
  Partials      130      130
Impacted Files Coverage Δ
pkg/client/interfaces.go 82.5% <ø> (ø) ⬆️
pkg/plugin/manifest/manifest.go 2.94% <0%> (-0.77%) ⬇️
pkg/plugin/driver/base.go 60.12% <0%> (-23.95%) ⬇️
cmd/sonobuoy/app/gen_plugin_def.go 91.66% <100%> (+0.75%) ⬆️
pkg/plugin/driver/daemonset/daemonset.go 64.47% <100%> (-1.78%) ⬇️
pkg/plugin/driver/job/job.go 72.41% <100%> (-0.7%) ⬇️
cmd/sonobuoy/app/args.go 89.69% <100%> (+0.32%) ⬆️
pkg/client/gen.go 84% <100%> (+0.21%) ⬆️
pkg/plugin/manifest/serializer.go 37.93% <100%> (+1.08%) ⬆️
cmd/sonobuoy/app/gen.go 54.01% <50%> (-0.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cce037...24075a3. Read the comment docs.

@zubron zubron force-pushed the zubron:customise-pod-options-add-pod-spec-809 branch from 766f1a9 to 517196f Aug 14, 2019
@johnSchnake

This comment has been minimized.

Copy link
Contributor

johnSchnake commented Aug 15, 2019

Where are you at on this? Still marked as a draft but I don't see a lot that really needs work. Maybe a double check over key pieces of logic and ensure they are tested (but I don't want to stall it just to add tests for trivial logic)

@zubron

This comment has been minimized.

Copy link
Member Author

zubron commented Aug 15, 2019

@johnSchnake I was just adding a few more tests to some of the parts that I knew weren't covered such as the job/daemonset driver packages. I should be done with it very shortly though and will ping you when ready.

@zubron zubron force-pushed the zubron:customise-pod-options-add-pod-spec-809 branch from 517196f to d08d960 Aug 15, 2019
@zubron zubron marked this pull request as ready for review Aug 15, 2019
@zubron zubron force-pushed the zubron:customise-pod-options-add-pod-spec-809 branch from d08d960 to 446ad4f Aug 15, 2019
This change introduces the ability to override PodSpec options for
plugins. A new flag `--show-default-podspec` has been added to the
`gen` and `gen plugin` commands. When used, Sonobuoy will include the
default PodSpec options for the given plugin driver in the resulting
yaml. This allows users to change these values or they can provide their
own. If one is provided in the plugin definition, Sonobuoy will use that
as is, otherwise it will use the default.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
@zubron zubron force-pushed the zubron:customise-pod-options-add-pod-spec-809 branch from 446ad4f to 24075a3 Aug 15, 2019
@zubron zubron merged commit 0fb9865 into vmware-tanzu:master Aug 15, 2019
7 checks passed
7 checks passed
Header rules - sonobuoy-live No header rules processed
Details
Pages changed - sonobuoy-live 127 new files uploaded
Details
Redirect rules - sonobuoy-live No redirect rules processed
Details
Mixed content - sonobuoy-live No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/sonobuoy-live/deploy-preview Deploy preview ready!
Details
signed-off-by Commit has Signed-off-by
Details
@zubron zubron deleted the zubron:customise-pod-options-add-pod-spec-809 branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.