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

default semver constraint for empty string #552

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zywillc
Copy link

@zywillc zywillc commented Apr 11, 2023

Closes #556

This PR is to fix 2 issues:

  1. current tags filtering behavior will skip semver constraint for prereleases versions (although they are valid semver versions) when there is no explicit constraint parsed from image tags (i.e. empty string) which is inconsistent with the document description.
    https://github.com/argoproj-labs/argocd-image-updater/blob/master/pkg/image/version.go#L135-L147

  2. some complex prerelease constraint rules are actually not supported untilsemver > 3.2.0

@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.85%. Comparing base (4f21ade) to head (5625cdb).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
+ Coverage   74.82%   74.85%   +0.03%     
==========================================
  Files          31       31              
  Lines        3912     3917       +5     
==========================================
+ Hits         2927     2932       +5     
  Misses        850      850              
  Partials      135      135              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution and apologies that it took so long to come back to this PR.

Would you mind to rebase this PR (some of the lint changes have already been merged through another PR), and to supply a unit test for the * case?

Thanks a lot!

@chengfang
Copy link
Collaborator

chengfang commented Oct 21, 2024

@zywillc I've rebased and the result is 1 file change in version.go. Can you review it, and maybe add some unit tests?

func Test_LatestVersion(t *testing.T) {
seems to be a good place for such tests.

@chengfang
Copy link
Collaborator

I did some manual testing with the current master, WITHOUT changes in this PR, and here are my findings:

  • with no constraint ("" constraint), image updater picks the latest version including pre-release versions. If a new pre-release version exists, it will be selected as the latest one instead of any older stable version.
dist/argocd-image-updater test quay.io/cfang/argocd-operator --update-strategy semver
INFO[0002] latest image according to constraint is quay.io/cfang/argocd-operator:1.2.0-rc1
  • with constraint "*", image updater chooses from stable versions only.
dist/argocd-image-updater test quay.io/cfang/argocd-operator --update-strategy semver --semver-constraint="*"
INFO[0004] latest image according to constraint is quay.io/cfang/argocd-operator:1.1.0
  • with constraint "x.x.x-0", image updater chooses from both stable and pre-releases versions. If a new pre-release version exists, it will be selected as the latest one instead of any older stable version.
dist/argocd-image-updater test quay.io/cfang/argocd-operator --update-strategy semver --semver-constraint="x.x.x-0"
INFO[0001] latest image according to constraint is quay.io/cfang/argocd-operator:1.2.0-rc1

I think the current behavior already matches the doc and your expectation, and no changes are needed.

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.

semver strategy is NOT working as the document claimed
4 participants