Skip to content

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

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

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 added 4 commits July 9, 2023 09:11
Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Signed-off-by: KS. Yim <ks.yim@linecorp.com>
@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 <ks.yim@linecorp.com>
@ks-yim ks-yim force-pushed the fix-git-write-back-override branch from 24246a4 to 4a9f69d Compare July 9, 2023 00:14
Signed-off-by: KS. Yim <ks.yim@linecorp.com>
@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
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 <ks.yim@linecorp.com>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add helm override commit test

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* lint

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* fix shadowed err

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

---------

Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Co-authored-by: KS. Yim <ks.yim@linecorp.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>
jessebye pushed a commit to jessebye/argocd-image-updater that referenced this pull request Sep 1, 2023
Signed-off-by: satoru <satorulogic@gmail.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

docs: Fixed typo (argoproj-labs#589)

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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

Signed-off-by: satoru <satorulogic@gmail.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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

* ci: Fix codegen

Signed-off-by: jannfis <jann@mistrust.net>

* Fix tar call

Signed-off-by: jannfis <jann@mistrust.net>

---------

Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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

* Fix original override not respected

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add helm override commit test

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* lint

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* fix shadowed err

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

---------

Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Co-authored-by: KS. Yim <ks.yim@linecorp.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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 <8467862+jessebye@users.noreply.github.com>

* fix deps and tests

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

* fix spelling

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

---------

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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

Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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 <myc.monad@gmail.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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 <flozzone@gmail.com>

* Update golangci-lint to 1.52.2.

Signed-off-by: flozzone <flozzone@gmail.com>

* Replace deprecated golangci linters with 'unused' linter.

Signed-off-by: flozzone <flozzone@gmail.com>

* Fix Goimport issues.

Signed-off-by: flozzone <flozzone@gmail.com>

---------

Signed-off-by: flozzone <flozzone@gmail.com>
Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix: support ocischema.DeserializedImageIndex in registry client

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix test

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix: update go mods to use newer argocd app definition

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix deps and tests

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

merge master

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix go mods

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

refactor: use shared function to reduce duplication

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix: update go mods to use newer argocd app definition

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix deps and tests

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

merge master

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

fix go mods

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

Fix after rebase

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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

Signed-off-by: satoru <satorulogic@gmail.com>

docs: Fixed typo (argoproj-labs#589)

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

Signed-off-by: satoru <satorulogic@gmail.com>

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

* ci: Fix codegen

Signed-off-by: jannfis <jann@mistrust.net>

* Fix tar call

Signed-off-by: jannfis <jann@mistrust.net>

---------

Signed-off-by: jannfis <jann@mistrust.net>

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

* Fix original override not respected

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add helm override commit test

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* lint

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* fix shadowed err

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

---------

Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Co-authored-by: KS. Yim <ks.yim@linecorp.com>

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 <8467862+jessebye@users.noreply.github.com>

* fix deps and tests

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

* fix spelling

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

---------

Signed-off-by: Jesse Bye <8467862+jessebye@users.noreply.github.com>

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

Signed-off-by: Viacheslav Sychov <viacheslav.sychov@gmail.com>

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 <myc.monad@gmail.com>
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 <ks.yim@linecorp.com>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add helm override commit test

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* lint

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* fix shadowed err

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

---------

Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Co-authored-by: KS. Yim <ks.yim@linecorp.com>
Signed-off-by: Francesc Arbona <francesc.arbona@global.ntt>
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 <ks.yim@linecorp.com>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add helm override commit test

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* lint

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* fix shadowed err

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

---------

Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Co-authored-by: KS. Yim <ks.yim@linecorp.com>
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 <ks.yim@linecorp.com>

* Add writeOverrides unittest

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* Add helm override commit test

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* lint

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

* fix shadowed err

Signed-off-by: KS. Yim <ks.yim@linecorp.com>

---------

Signed-off-by: KS. Yim <ks.yim@linecorp.com>
Co-authored-by: KS. Yim <ks.yim@linecorp.com>
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