-
Notifications
You must be signed in to change notification settings - Fork 25
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 <maufart@redhat.com>
@@ -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 <maufart@redhat.com>
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 <maufart@redhat.com>
* 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 <maufart@redhat.com> * Force runnerImg env lookup Signed-off-by: Marek Aufart <maufart@redhat.com> * Fix env config load override Signed-off-by: Marek Aufart <maufart@redhat.com> * Revert "Fix env config load override" This reverts commit 2950638. Signed-off-by: Marek Aufart <maufart@redhat.com> * Update env settings with field values Signed-off-by: Marek Aufart <maufart@redhat.com> * Adding RUNNER_IMG setting test Signed-off-by: Marek Aufart <maufart@redhat.com> * Revert Settings changes Signed-off-by: Marek Aufart <maufart@redhat.com> --------- Signed-off-by: Marek Aufart <maufart@redhat.com>
Adding validation for analysis source and target labels.
Fixes: #176