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 issue with force-update and semver #192

Merged
merged 3 commits into from Apr 15, 2021

Conversation

iamnoah
Copy link
Contributor

@iamnoah iamnoah commented Apr 14, 2021

I realize the test might be a little strange, but I assumed my issue had something to do with customize being used to change the image name so I started there and finally narrowed it down to the version constraint being used as a version.

@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #192 (4983e5c) into master (93d76f9) will increase coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   66.77%   66.99%   +0.22%     
==========================================
  Files          20       20              
  Lines        1502     1503       +1     
==========================================
+ Hits         1003     1007       +4     
+ Misses        406      405       -1     
+ Partials       93       91       -2     
Impacted Files Coverage Δ
pkg/argocd/argocd.go 63.28% <100.00%> (+1.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93d76f9...4983e5c. Read the comment docs.

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.

Hey @iamnoah - thanks for this bug fix, and also for including a unit test for it!

I think we can simplify the fix, please see below. WDYT?

Comment on lines 434 to 443
// code consuming this list assumes the tag is valid, but for a force update, we may only have a constraint which isn't a valid tag
// if using semver, try to parse the tag and remove it if it fails
if strategy, set := annotations[fmt.Sprintf(common.UpdateStrategyAnnotation, img.ImageAlias)]; (!set ||
image.ParseUpdateStrategy(strategy) == image.VersionSortSemVer) && img.ImageTag != nil {
_, err := semver.NewVersion(img.ImageTag.TagName)
if err != nil {
log.Debugf("Dropping tag from force-update of image %s because %v", img, err)
img.ImageTag = &tag.ImageTag{}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we can simply set the tag to nil from what is specified in image-list, without having to perform checks for the update strategy or whether the tag can be correctly parsed as a semver.

If an image tag is nil in one of the images we check for, we explicitly create an empty tag here and handle that case appropriately here

So I think we can safely remove all the logic here, and just set img.ImageTag = nil here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I'll update shortly

@jannfis
Copy link
Contributor

jannfis commented Apr 14, 2021

Regarding the failing golangci-lint check, it will check for properly formatted import blocks. The logic it follows is to check

  • whether the imports are correctly grouped (groups are defined by an empty line within the imports)
  • whether each group is sorted alphabetically

Import groups are Golang standard library, own package, and external packages, so basically:

import (
  // standard lib
  "fmt"
  "time"

  // own package imports
  "github.com/argoproj-labs/argocd-image-updater/pkg/image

  // external packages
  "github.com/Masterminds/semver"
)

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.

LGTM, thank you @iamnoah !

@jannfis jannfis merged commit 37433e4 into argoproj-labs:master Apr 15, 2021
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.

None yet

2 participants