-
Notifications
You must be signed in to change notification settings - Fork 27
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
🐛 Validate source and target labels #243
Conversation
Adding validation for analysis source and target labels. Fixes: konveyor#176 Signed-off-by: Marek Aufart <[email protected]>
@@ -113,7 +114,7 @@ func NewAnalyzeCmd(log logr.Logger) *cobra.Command { | |||
return err | |||
} | |||
} | |||
err := analyzeCmd.Validate() | |||
err := analyzeCmd.Validate(cmd.Context()) |
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.
Adding context to Validate() for non-trivial validations (running analyzer container for listing sources/targets in this case).
// Validate source labels | ||
if len(a.sources) > 0 { | ||
var sourcesRaw bytes.Buffer | ||
a.fetchLabels(ctx, true, false, &sourcesRaw) |
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.
Re-used ListLabels extracted method body to have the share the logic for fetching sources/targets from analyzer. However, this doesn't look as nice as I'd like, looking on update.
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.
Is there some way to use the slice created here https://github.com/konveyor/kantra/blob/main/cmd/analyze.go#L454 to return that in validate, and then re-use it to list everything after?
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.
@eemcmullan I guess it won't be possible because validation runs locally, list commands work in container
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 approach makes sense to me, don't know if there's a better way, thanks!
// Validate source labels | ||
if len(a.sources) > 0 { | ||
var sourcesRaw bytes.Buffer | ||
a.fetchLabels(ctx, true, false, &sourcesRaw) |
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.
@eemcmullan I guess it won't be possible because validation runs locally, list commands work in container
Fixing fmt.Fprintln to correctly recognize first argument as a writer buffer. Bug was introduced in konveyor#243 Should fix kantra CI. Signed-off-by: Marek Aufart <[email protected]>
Fixing fmt.Fprintln to correctly recognize first argument as a writer buffer. Bug was introduced in konveyor#243 Should fix kantra CI. Signed-off-by: Marek Aufart <[email protected]>
* Fix labels validation print Fixing fmt.Fprintln to correctly recognize first argument as a writer buffer. Bug was introduced in #243 Should fix kantra CI. Signed-off-by: Marek Aufart <[email protected]> * Force runnerImg env lookup Signed-off-by: Marek Aufart <[email protected]> * Fix env config load override Signed-off-by: Marek Aufart <[email protected]> * Revert "Fix env config load override" This reverts commit 2950638. Signed-off-by: Marek Aufart <[email protected]> * Update env settings with field values Signed-off-by: Marek Aufart <[email protected]> * Adding RUNNER_IMG setting test Signed-off-by: Marek Aufart <[email protected]> * Revert Settings changes Signed-off-by: Marek Aufart <[email protected]> --------- Signed-off-by: Marek Aufart <[email protected]>
Adding validation for analysis source and target labels.
Fixes: #176