-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
respect arch param when sort=semver in [DockerVersion] badge #10905
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: master
Are you sure you want to change the base?
Conversation
export { | ||
sizeDataNoTagSemVerSort, | ||
versionDataNoTagDateSort, | ||
versionPagedDataNoTagDateSort, | ||
versionDataNoTagSemVerSort, | ||
versionDataWithTag, | ||
versionDataWithVaryingArchitectures, | ||
versionDataWithArchSpecificVersions, |
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.
Today in "naming things is hard"
I've not seen this one before and it gave me a good chuckle so thanks for sharing 😄 I feel like the combination of the query param values is currently a silent error of sorts in that As such I feel like we should do something and between this proposed change or an alternative (e.g. return an error badge when the user provides a value for Perhaps we could reduce the fallout by changing the defaulting behavior for |
Thanks for the suggestions. I had another look over the code and considered them. I don't think we should make it an error if the arch param is used because there's not really a good reason why you can't use that param, other than we implemented it wrong. My instinct was that your second suggestion might be better. If we were going to go down the road of saying that we look at all images of all architectures when the Even though the change in this PR makes a behaviour change, the change it makes (arch param works, and if you don't specify it the default is |
SGTM! |
Is that SGTM lets merge this, or SGTM lets do something else? |
Refs #10904
This is a bit of a "remove spacebar heating" change
I think my reading here is that this change fundamentally brings the code into line with the documented behaviour, so we could consider it a non-breaking change.
That said, this probably is going to change the result of this badge for some projects in the wild which rely on the fact the semver variant ignores architectures (because it assumes projects follow the more normal pattern where you push multiple architectures to a single version tag).
As such, this is a bit of a debatable one. I can see that we could equally choose to not merge this to preserve backwards compatibility and document more clearly the existing behaviour.