-
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 piping input to sonobuoy run #925
Conversation
@@ -88,15 +96,6 @@ func submitSonobuoyRun(cmd *cobra.Command, args []string) { | |||
os.Exit(1) | |||
} | |||
|
|||
plugins := make([]string, len(runCfg.Config.PluginSelections)) |
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.
I just removed this section. I could have gated it by checking if the config was non-empty but it just didnt seem like a valuable piece of code.
They can immediately check the plugins via sonobuoy status
if they want and could have checked it beforehand with sonobuoy gen. Now they can even tee the output from sonobuoy gen to a file and run it with sonobuoy run so they have a true file of what they're doing.
Codecov Report
@@ Coverage Diff @@
## master #925 +/- ##
==========================================
+ Coverage 47.47% 47.59% +0.11%
==========================================
Files 76 76
Lines 5156 5175 +19
==========================================
+ Hits 2448 2463 +15
- Misses 2559 2562 +3
- Partials 149 150 +1
Continue to review full report at Codecov.
|
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.
I'm really happy to have this feature, thank you! 😄
cmd/sonobuoy/app/run.go
Outdated
@@ -46,20 +45,30 @@ func RunFlagSet(cfg *runFlags) *pflag.FlagSet { | |||
AddSkipPreflightFlag(&cfg.skipPreflight, runset) | |||
AddRunWaitFlag(&cfg.wait, runset) | |||
AddWaitOutputFlag(&cfg.waitOutput, runset, SilentOutputMode) | |||
runset.StringVarP( | |||
&cfg.genFile, "file", "f", "", | |||
"If set, loads the file as if it were the output to sonobuoy gen. Set to `-` to read from stdin.", |
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.
output from sonobuoy gen?
pkg/client/run.go
Outdated
var manifest []byte | ||
var err error | ||
if len(cfg.GenFile) != 0 { | ||
if err := cfg.Validate(); err != nil { |
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.
This config validation should take place outside of this conditional. It primarily validates the GenConfig
contained within so by having it in this branch, it's not validated before being used in GenerateManifest
.
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.
Thanks for the catch; I had swapped the conditional so you're right, the validate method needed moved to the other branch.
When the GenFile is given (instead of a genconfig) we dont do validation.
Just to be clear, the reason it is coded like this is because it is quite difficult to go from the YAML to a genConfig (various objects that need deserialized and so many options that could be tweaked at any given place or maybe are even configurable in our data format).
Often a user wants to use `sonbouoy gen` to generate some YAML and then modify it using tools like jq or yq. We even have this workflow documented and the user has to, end the end, pipe the data into `kubectl apply -f`. `sonobuoy run` is just a shorthand for `sonobuoy gen` + execute with wait logic; it makes sense to allow loading a file or piping the data from stdin. This PR: - adds a --file, -f flag to `sonobuoy run` - if the value is `-` it will load from stdin, erroring if it is not connected to a terminal. It does not add a unit test for this since the `sonobuoy run` logic is difficult to test in a unit test since it wants to talk to a real cluster. We may consider adding this to integration tests but it is mostly just input transformation and leaves little room for error. Fixes #920 Signed-off-by: John Schnake <[email protected]>
What this PR does / why we need it:
Often a user wants to use
sonbouoy gen
to generate some YAML andthen modify it using tools like jq or yq. We even have this workflow
documented and the user has to, end the end, pipe the data into
kubectl apply -f
.sonobuoy run
is just a shorthand forsonobuoy gen
+ execute withwait logic; it makes sense to allow loading a file or piping the data
from stdin.
This PR:
sonobuoy run
-
it will load from stdin, erroring if it is notconnected to a terminal.
It does not add a unit test for this since the
sonobuoy run
logicis difficult to test in a unit test since it wants to talk to a real cluster.
We may consider adding this to integration tests but it is mostly just input
transformation and leaves little room for error.
Which issue(s) this PR fixes
Fixes #920
Special notes for your reviewer:
You can easily test this locally since it is just client side behavior. Try things like this:
Release note: