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

allow image updates to be forced if container not present #181 #189

Merged
merged 2 commits into from Apr 13, 2021

Conversation

iamnoah
Copy link
Contributor

@iamnoah iamnoah commented Apr 11, 2021

Supports #187. I've tested against my deployment of a PodTemplate it works wonderfully :)

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #189 (68dcba4) into master (f1cc8e4) will increase coverage by 0.01%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   67.10%   67.12%   +0.01%     
==========================================
  Files          19       19              
  Lines        1456     1466      +10     
==========================================
+ Hits          977      984       +7     
- Misses        388      391       +3     
  Partials       91       91              
Impacted Files Coverage Δ
pkg/image/image.go 89.41% <0.00%> (-3.28%) ⬇️
pkg/argocd/argocd.go 61.65% <100.00%> (+1.34%) ⬆️

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 f1cc8e4...68dcba4. 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.

Thank you @iamnoah for taking the time and effort to submit this new feature!

Your PR looks awesome, and I have no formal request for change. Just some items up for discussion - please see below.

Comment on lines 190 to 192
func parseImageList(updateImage string) []string {
return strings.Split(updateImage, ",")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this becomes externalized in its own method. Maybe it would make sense to also pull in the white-space trimming into this method, what do you think? Also, I think it could be worth to spend this method its own unit test :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once I pulled in the white space, I noticed the next step was to convert to a ContainerImage so I pulled that in as well. I just focused tests on the string parsing since NewFromIdentifier seems well covered.

pkg/argocd/argocd.go Show resolved Hide resolved
@@ -56,6 +56,26 @@ func Test_GetImagesFromApplication(t *testing.T) {
imageList := GetImagesFromApplication(application)
assert.Empty(t, imageList)
})

t.Run("Get list of images from application that has force-update", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests are always appreciated, thank you! 👍

@@ -61,6 +61,21 @@ of the [Semver library](https://github.com/Masterminds/semver) we're using.
[filtering tags](#filtering-tags)
below.


### Forcing Image Updates
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for documentation update :)


func Test_parseImageList(t *testing.T) {
assert.Equal(t, []string{"foo", "bar"}, parseImageList(" foo, bar ").Originals())
// should whitespace inside the spec be preserved?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think whitespace stripping should be done here, but let's do that in another PR:

func getImageTagFromIdentifier(identifier string) (string, string, *tag.ImageTag) {

@jannfis jannfis merged commit 0257771 into argoproj-labs:master Apr 13, 2021
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 !

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

3 participants