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

🐛 Fix labels validation print #251

Merged
merged 7 commits into from
Jun 3, 2024

Conversation

aufi
Copy link
Member

@aufi aufi commented May 31, 2024

Fixing fmt.Fprintln to correctly recognize first argument as a writer buffer. Bug was introduced in my PR #243

This should fix kantra CI.

@aufi
Copy link
Member Author

aufi commented May 31, 2024

I believe the CI uses "previous" kantra image, that was reason it didn't fail on original PR and looks red on this PR.

Works with locally built image based on this PR.

@pranavgaikwad
Copy link
Contributor

@aufi I don't think CI uses previous image, we build one in one of the steps unless I am missing something.

@pranavgaikwad
Copy link
Contributor

I think we need to update the parsing logic to accomodate for weirdness in different shells (thinking about windows too). Can we just match a regex for label value format on every line of output https://github.com/konveyor/analyzer-lsp/blob/82b3f8b673cbfc178259faf6e35846bc0534d409/engine/labels/labels.go#L21

@aufi
Copy link
Member Author

aufi commented May 31, 2024

@aufi I don't think CI uses previous image, we build one in one of the steps unless I am missing something.

From what I can see locally, the RUNNER_IMG is not propagated to containers kantra executes

$ RUNNER_IMG=kantra-labels go run main.go  analyze --list-targets --log-level 99 # containers still use quay.io/...kantra

Will look on that to be sure I'm not wrong.

@aufi
Copy link
Member Author

aufi commented May 31, 2024

I think we need to update the parsing logic to accomodate for weirdness in different shells (thinking about windows too). Can we just match a regex for label value format on every line of output https://github.com/konveyor/analyzer-lsp/blob/82b3f8b673cbfc178259faf6e35846bc0534d409/engine/labels/labels.go#L21

Sounds good, just question what if a line would have more than one match of the regex? (fail I'd suggest)

@eemcmullan
Copy link
Collaborator

@pranavgaikwad I noticed the same issue as @aufi while I was debugging #247 where the listLabels command uses the current image at quay.io/konveyor/kantra:latest and not a fresh build of runnerImage. Although I built and set a new runnerImage, I had to override these values locally https://github.com/konveyor/kantra/blob/main/cmd/version.go#L11-L12 for it to finally work.

@pranavgaikwad
Copy link
Contributor

pranavgaikwad commented May 31, 2024

@eemcmullan looks like this is always setting the default value?

kantra/cmd/settings.go

Lines 89 to 96 in 52f87d7

img := strings.TrimSuffix(RunnerImage, fmt.Sprintf(":%v", Version))
updatedImg := fmt.Sprintf("%v:%v", img, Version)
err := os.Setenv("RUNNER_IMG", updatedImg)
if err != nil {
return err
}
return nil

Shouldn't user input values take precedence over defaults?

@@ -85,8 +85,13 @@ func (c *Config) trySetDefaultPodmanBin(file string) (found bool, err error) {
}

func (c *Config) loadRunnerImg() error {
// TODO(maufart): ensure Config struct works/parses it values from ENV and defaults correctly
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick friday-afternoon solution, will continue/follow-up on Monday. Seems the Config mechanism is not setup correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problem affects more config options, submitted an issue #252 expecting it is a follow-up, but changing things unrelated to this PR.

aufi added 4 commits May 31, 2024 18:00
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]>
Signed-off-by: Marek Aufart <[email protected]>
Signed-off-by: Marek Aufart <[email protected]>
This reverts commit 2950638.

Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi force-pushed the fix-labels-validation-print branch from 222d5c4 to af1e533 Compare May 31, 2024 16:00
@aufi
Copy link
Member Author

aufi commented Jun 3, 2024

There was a conflict with package-level variables from https://github.com/konveyor/kantra/blob/main/cmd/version.go#L12-L16 partially overlapping Settings fields names. Updated Settings to load env with tags (default) as the first step and adjusting it in custom methods as the second one (and use variables within the Settings struct, not outsite).

Signed-off-by: Marek Aufart <[email protected]>
@aufi aufi force-pushed the fix-labels-validation-print branch from 55c0c5a to 5de6055 Compare June 3, 2024 13:30
@aufi aufi merged commit 6d5fd4c into konveyor:main Jun 3, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants