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: adding commonAnnotations #4613

Merged
merged 5 commits into from
Oct 29, 2020
Merged

Conversation

igaskin
Copy link
Member

@igaskin igaskin commented Oct 20, 2020

optional k/v map to add annotations via kustomize.
#4603

dont mind me, just copy pasting some code...

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I've signed the CLA and my build is green (troubleshooting builds).

@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #4613 into master will decrease coverage by 0.05%.
The diff coverage is 76.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4613      +/-   ##
==========================================
- Coverage   41.10%   41.05%   -0.06%     
==========================================
  Files         127      127              
  Lines       17367    17391      +24     
==========================================
+ Hits         7139     7140       +1     
- Misses       9202     9228      +26     
+ Partials     1026     1023       -3     
Impacted Files Coverage Δ
cmd/argocd/commands/app.go 6.50% <68.75%> (+0.41%) ⬆️
util/kustomize/kustomize.go 64.28% <83.33%> (+2.00%) ⬆️
pkg/apis/application/v1alpha1/types.go 57.98% <100.00%> (+0.05%) ⬆️
controller/appcontroller.go 50.56% <0.00%> (-1.92%) ⬇️

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 dd856e1...484d9dc. Read the comment docs.

Copy link
Member

@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 @igaskin! Change looks good so far, with two small review comments below.

Also, the CLI has an option for adding/modifying commonLabels, which I feel should be duplicated for commonAnnotations as well (somewhere here:

command.Flags().StringArrayVar(&opts.kustomizeCommonLabels, "kustomize-common-label", []string{}, "Set common labels in Kustomize")
)

I haven't had a look at the UI yet, does it provide means to add/edit commonLabels? If yes, maybe we should also give it a go for commonAnnotations :)

pkg/apis/application/v1alpha1/types.go Outdated Show resolved Hide resolved
util/kustomize/kustomize.go Outdated Show resolved Hide resolved
also correcting kustomize cli flags to respect multiple options

argoproj#4613
@igaskin
Copy link
Member Author

igaskin commented Oct 21, 2020

Thank you @igaskin! Change looks good so far, with two small review comments below.

Also, the CLI has an option for adding/modifying commonLabels, which I feel should be duplicated for commonAnnotations as well (somewhere here:

command.Flags().StringArrayVar(&opts.kustomizeCommonLabels, "kustomize-common-label", []string{}, "Set common labels in Kustomize")

)
I haven't had a look at the UI yet, does it provide means to add/edit commonLabels? If yes, maybe we should also give it a go for commonAnnotations :)

It doesn't appear that the UI has a means to add/edit commonLabels, but it probably should. Maybe that should warrant a separate PR?

image

I ran the kustomize_test.go test locally, and confirmed that annotations were added as expected. There was also a bug in the setKustomizeOpt method that I addressed by adding some nil checks. Prior to this it would only honor the last kustomize option.

@jannfis
Copy link
Member

jannfis commented Oct 23, 2020

It doesn't appear that the UI has a means to add/edit commonLabels, but it probably should. Maybe that should warrant a separate PR?

Yes agreed, this could be done in a separate PR.

Can you please re-sync your branch with latest master and re-run codegen to resolve conflict in swagger.json?

Copy link
Member

@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.

Awesome, thank you @igaskin

LGTM

@jannfis jannfis merged commit 7f0ffb4 into argoproj:master Oct 29, 2020
@igaskin igaskin deleted the user/igaskin/4603 branch October 30, 2020 03:35
terrycorley pushed a commit to terrycorley/argo-cd that referenced this pull request Nov 3, 2020
…zation-generators

* 'master' of github.com:argoproj/argo-cd:
  fix: RevisionFormField component crashes in 'refs' API returns no tags (argoproj#4735)
  docs: add Opensurvey to USERS.md (argoproj#4727)
  docs: correct parameters usage in CLI (argoproj#4725)
  fix: Repo-server has silent unmarshalling errors leading to empty applications (argoproj#4423) (argoproj#4708)
  fix: inject artificial delay between sync waves to better support health assessments (argoproj#4715)
  fix: exclude files listed under exclusions (argoproj#4686)
  feat: support resource actions on CRDs that use status subresources (argoproj#4690)
  feat: Add autocomplete for repo Revisions (argoproj#4645) (argoproj#4713)
  fix: webhook don't refresh apps pointing to HEAD (argoproj#4717)
  feat: Add support for ExecProvider cluster auth (argoproj#4600) (argoproj#4710)
  fix: adding helm values file in New App (argoproj#4635)
  docs: Instructions on `make verify-kube-connect` step when using k3d (argoproj#4687)
  feat:  Annotation based app paths detection in webhooks (argoproj#4699)
  fix: adding commonAnnotations for Kustomize (argoproj#4613)
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