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

feat: Respect original parameter overrides with git write-back #573

Merged
merged 6 commits into from
Jul 9, 2023

Conversation

ks-yim
Copy link
Contributor

@ks-yim ks-yim commented Jun 9, 2023

Modification:

Make image-updater respect the original parameter overrides written in .argocd-source-<appName>.yaml.

Motivation:

Currently, the original overridden parameters stored in .argocd-source-<appName>.yaml
are not respected when image-updater writes back its change to git.

Imagine that a user has originally defined .argocd-source-<appName>.yaml like below

helm:
  parameters:
  - name: some.prop
    value: some-value
    forcestring: true

...and image-updater wants to write back the additional overrides below

  • foo.image.repository: foo
  • foo.image.tag: 2.0.0

The user may want the edited file to be like...

helm:
  parameters:
  - name: some.prop
    value: some-value
    focestring: true
  - name: foo.image.repository
    value: foo
    forcestring: true
  - name: foo.image.tag
    value: 2.0.0
    forcestring: true

... but what's actually committed to git is the following and it deletes the original user-defined parameter overrides.

helm:
  parameters:
  - name: foo.image.repository
    value: foo
    forcestring: true
  - name: foo.image.tag
    value: 2.0.0
    forcestring: true

And this image-updater behavior is a suspected cause of #562 and the likes
as image-updater ignores the tags for an image that it failed to fetch when
there are multiple updatable images defined in an Application and generate
the parameter overrides without the ignored image.
If only a part of image tags were successfully fetched from the image registry
due to unstable network, image-updater may write a broken parameter override
file to git.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2023

Codecov Report

Merging #573 (4a9f69d) into master (5366cc1) will decrease coverage by 0.24%.
The diff coverage is 41.02%.

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
- Coverage   65.86%   65.63%   -0.24%     
==========================================
  Files          22       22              
  Lines        2045     2069      +24     
==========================================
+ Hits         1347     1358      +11     
- Misses        569      577       +8     
- Partials      129      134       +5     
Impacted Files Coverage Δ
pkg/argocd/update.go 66.05% <35.71%> (-3.75%) ⬇️
pkg/argocd/git.go 66.82% <54.54%> (+1.98%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ks-yim ks-yim changed the title Respect original override parameters with git write-back Respect original parameter overrides with git write-back Jun 9, 2023
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.

Nice one, thank you!

The linter complained, and it turns out your code shadows err in one place. Please fix it, otherwise LGTM :)

override, err = yaml.Marshal(newParams)
break
}
err := yaml.Unmarshal(originalData, &params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err := yaml.Unmarshal(originalData, &params)
err = yaml.Unmarshal(originalData, &params)

@jannfis jannfis changed the title Respect original parameter overrides with git write-back feat: Respect original parameter overrides with git write-back Jul 6, 2023
@jannfis
Copy link
Contributor

jannfis commented Jul 6, 2023

Oh, and please make sure the DCO passes by signing off your commits

@ks-yim ks-yim force-pushed the fix-git-write-back-override branch 2 times, most recently from f22b5f9 to 0c5761f Compare July 8, 2023 23:39
@ks-yim ks-yim force-pushed the fix-git-write-back-override branch from 66f0e2b to 24246a4 Compare July 9, 2023 00:12
Signed-off-by: KS. Yim <[email protected]>
@ks-yim ks-yim force-pushed the fix-git-write-back-override branch from 24246a4 to 4a9f69d Compare July 9, 2023 00:14
@ks-yim
Copy link
Contributor Author

ks-yim commented Jul 9, 2023

@jannfis
Thanks for taking your time to review this!

The issue described in the PR description has actually caused a disruption to our production(insignificant, though) workload so it would be very nice if I can ship this into the next release.

Resolved the variable shadowing issue and signed-off all my commits included in the PR.
I tried to make it pass Spell checking and Run codegen by merging the latest master but had no luck. 😅

@jannfis
Copy link
Contributor

jannfis commented Jul 9, 2023

Thanks for looking into it, @ks-yim!

I have just merged #590 , so if you rebase your PR, the codegen step should pass.

I'm aiming to release v0.13.0 after #578 has been merged, however, it needs some attention from the submitter.

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. Going to merge it without rebase required, because the codegen error is negligible.

@jannfis jannfis merged commit f5419f6 into argoproj-labs:master Jul 9, 2023
@ks-yim ks-yim deleted the fix-git-write-back-override branch July 12, 2023 07:19
jwhy89 pushed a commit to jwhy89/argocd-image-updater that referenced this pull request Aug 17, 2023
…roj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>
Signed-off-by: Jarvis Yang <[email protected]>
jessebye pushed a commit to jessebye/argocd-image-updater that referenced this pull request Sep 1, 2023
…roj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>
jessebye pushed a commit to jessebye/argocd-image-updater that referenced this pull request Sep 1, 2023
Signed-off-by: satoru <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

docs: Fixed typo (argoproj-labs#589)

Signed-off-by: Jesse Bye <[email protected]>

chore: Preallocate space for slices with known size (argoproj-labs#575)

Signed-off-by: satoru <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

ci: Fix codegen and update kustomize to post-2.0 (argoproj-labs#590)

* ci: Fix codegen

Signed-off-by: jannfis <[email protected]>

* Fix tar call

Signed-off-by: jannfis <[email protected]>

---------

Signed-off-by: jannfis <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

feat: Respect original parameter overrides with git write-back (argoproj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

chore: Update to newer argocd version for better API compatibility (argoproj-labs#594)

* fix: update go mods to use newer argocd app definition

Signed-off-by: Jesse Bye <[email protected]>

* fix deps and tests

Signed-off-by: Jesse Bye <[email protected]>

* fix spelling

Signed-off-by: Jesse Bye <[email protected]>

---------

Signed-off-by: Jesse Bye <[email protected]>

chore(deps): upgrade dependencies for fix vulnerabilities (argoproj-labs#599)

Signed-off-by: Viacheslav Sychov <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

fix: tighten securityContext to comply with restricted PSS (argoproj-labs#600)

makes argocd-image-updater compatible with restricted Pod Security Standard

Signed-off-by: Takeo Sawada <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

feat: Add possibility to specify write-back GIT repository as annotation (argoproj-labs#424)

* Add possibility to specify write-back GIT repository as annotation.

Signed-off-by: flozzone <[email protected]>

* Update golangci-lint to 1.52.2.

Signed-off-by: flozzone <[email protected]>

* Replace deprecated golangci linters with 'unused' linter.

Signed-off-by: flozzone <[email protected]>

* Fix Goimport issues.

Signed-off-by: flozzone <[email protected]>

---------

Signed-off-by: flozzone <[email protected]>
Signed-off-by: Jesse Bye <[email protected]>

fix: support ocischema.DeserializedImageIndex in registry client

Signed-off-by: Jesse Bye <[email protected]>

fix test

Signed-off-by: Jesse Bye <[email protected]>

fix: update go mods to use newer argocd app definition

Signed-off-by: Jesse Bye <[email protected]>

fix deps and tests

Signed-off-by: Jesse Bye <[email protected]>

merge master

Signed-off-by: Jesse Bye <[email protected]>

fix go mods

Signed-off-by: Jesse Bye <[email protected]>

refactor: use shared function to reduce duplication

Signed-off-by: Jesse Bye <[email protected]>

fix: update go mods to use newer argocd app definition

Signed-off-by: Jesse Bye <[email protected]>

fix deps and tests

Signed-off-by: Jesse Bye <[email protected]>

merge master

Signed-off-by: Jesse Bye <[email protected]>

fix go mods

Signed-off-by: Jesse Bye <[email protected]>

Fix after rebase

Signed-off-by: Jesse Bye <[email protected]>

chore: Fix spell checking config (argoproj-labs#577)

Signed-off-by: satoru <[email protected]>

docs: Fixed typo (argoproj-labs#589)

chore: Preallocate space for slices with known size (argoproj-labs#575)

Signed-off-by: satoru <[email protected]>

ci: Fix codegen and update kustomize to post-2.0 (argoproj-labs#590)

* ci: Fix codegen

Signed-off-by: jannfis <[email protected]>

* Fix tar call

Signed-off-by: jannfis <[email protected]>

---------

Signed-off-by: jannfis <[email protected]>

feat: Respect original parameter overrides with git write-back (argoproj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>

chore: Update to newer argocd version for better API compatibility (argoproj-labs#594)

* fix: update go mods to use newer argocd app definition

Signed-off-by: Jesse Bye <[email protected]>

* fix deps and tests

Signed-off-by: Jesse Bye <[email protected]>

* fix spelling

Signed-off-by: Jesse Bye <[email protected]>

---------

Signed-off-by: Jesse Bye <[email protected]>

chore(deps): upgrade dependencies for fix vulnerabilities (argoproj-labs#599)

Signed-off-by: Viacheslav Sychov <[email protected]>

fix: tighten securityContext to comply with restricted PSS (argoproj-labs#600)

makes argocd-image-updater compatible with restricted Pod Security Standard

Signed-off-by: Takeo Sawada <[email protected]>
xescab pushed a commit to xescab/argocd-image-updater that referenced this pull request Sep 8, 2023
…roj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>
Signed-off-by: Francesc Arbona <[email protected]>
dlactin pushed a commit to dlactin/argocd-image-updater that referenced this pull request May 9, 2024
…roj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>
sribiere-jellysmack pushed a commit to sribiere-jellysmack/argocd-image-updater that referenced this pull request Aug 13, 2024
…roj-labs#573)

* Fix original override not respected

Signed-off-by: KS. Yim <[email protected]>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <[email protected]>

* Add helm override commit test

Signed-off-by: KS. Yim <[email protected]>

* lint

Signed-off-by: KS. Yim <[email protected]>

* fix shadowed err

Signed-off-by: KS. Yim <[email protected]>

---------

Signed-off-by: KS. Yim <[email protected]>
Co-authored-by: KS. Yim <[email protected]>
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.

3 participants