-
-
Notifications
You must be signed in to change notification settings - Fork 512
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(localstack): more reliable legacy tag detection #2936
base: main
Are you sure you want to change the base?
fix(localstack): more reliable legacy tag detection #2936
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
4d651db
to
c8dd734
Compare
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 contribution, added a comment regarding the non-legacy tags
modules/localstack/localstack.go
Outdated
version = version[0:pos] | ||
} | ||
|
||
if version == "latest" || version == "s3" || version == "s3-latest" || version == "stable" { |
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.
suggestion: we could add an slice of valid non-legacy versions at the top of the file. Here we would just look up the used version to verify if it's legacy. Wdyt?
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.
Are you suggesting every legacy version is simply hard coded in a slice? It looks like there are 31 versions prior to v0.11
, plus the following unconventional tags that I have no idea about:
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.
Here's a much simpler implementation that actually works for all test cases except for the dummy "foo" one.
func isLegacyMode(image string) bool {
return strings.Compare(image, "localstack/localstack:0.11") < 0
}
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.
Are you suggesting every legacy version is simply hard coded in a slice? It looks like there are 31 versions prior to v0.11, plus the following unconventional tags that I have no idea aboutAre you suggesting every legacy version
No, just the ones in the if branch:
if version == "latest" || version == "s3" || version == "s3-latest" || version == "stable"
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.
Sure, done. I've also fixed the isVersion2
function along with unit tests.
I just noticed the |
299ea0c
to
c8dd734
Compare
c8dd734
to
09e009d
Compare
What does this PR do?
Improves the internal
isLegacyMode
function to handle all tag conventions used by localstack.Why is it important?
I'm currently unable to use the much smaller
localstack/localstack:s3-latest
image tag because it's incorrectly detected as a legacy version.How to test this PR
Unit test included.