-
-
Couldn't load subscription status.
- Fork 584
fix(kafka): strip architecture suffix from Kafka image tags for semver parsing #3276
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -219,6 +219,11 @@ func validateKRaftVersion(fqName string) error { | |||||
| version = "v" + version | ||||||
| } | ||||||
|
|
||||||
| // remove the architecture suffix | ||||||
| if strings.HasSuffix(version, ".amd64") || strings.HasSuffix(version, ".arm64") { | ||||||
| version = version[:strings.LastIndex(version, ".")] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mdelapenya How about something like this? But at this point, we might as well let them use the image by fixing the version.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given we are using semver libraries to get a valid version, then we should honor it. I think this is a good compromise, failing if the version is not following semver. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @asahasrabuddhe let's apply that fix, so we can merge this PR Thanks! |
||||||
| } | ||||||
|
|
||||||
| if semver.Compare(version, "v7.4.0") < 0 { // version < v7.4.0 | ||||||
| return fmt.Errorf("version=%s. KRaft mode is only available since version 7.4.0", version) | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -94,6 +94,15 @@ func TestValidateKRaftVersion(t *testing.T) { | |||||||||||||||||||
| image: "my-kafka:1.0.0", | ||||||||||||||||||||
| wantErr: false, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| { | ||||||||||||||||||||
| name: "Official: valid, with the amd64 architecture suffix", | ||||||||||||||||||||
| image: "confluentinc/confluent-local:7.5.9.amd64", | ||||||||||||||||||||
| wantErr: false, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| { | ||||||||||||||||||||
| name: "Official: valid, with the arm64 architecture suffix", | ||||||||||||||||||||
| image: "confluentinc/confluent-local:7.5.9.arm64", | ||||||||||||||||||||
| }, | ||||||||||||||||||||
|
Comment on lines
+102
to
+105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add missing The test case is incomplete - it's missing the Apply this diff to fix both issues: {
name: "Official: valid, with the arm64 architecture suffix",
image: "confluentinc/confluent-local:7.5.9.arm64",
+ wantErr: false,
},📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: Main pipeline[error] 103-103: golangci-lint: File is not properly formatted (gci) 🤖 Prompt for AI Agents |
||||||||||||||||||||
| { | ||||||||||||||||||||
| name: "lacks tag", | ||||||||||||||||||||
| image: "my-kafka", | ||||||||||||||||||||
|
|
||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.