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.

Sign up for GitHub

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

Jump to bottom

fix: configurable CMP tar exclusions (#9675) #9789

Merged
merged 7 commits into from
Jun 29, 2022
Merged

fix: configurable CMP tar exclusions (#9675) #9789

merged 7 commits into from
Jun 29, 2022

Conversation

notfromstatefarm
Copy link
Contributor

@notfromstatefarm notfromstatefarm commented Jun 26, 2022 •
edited

Signed-off-by: notfromstatefarm 86763948+notfromstatefarm@users.noreply.github.com

Fixes #9675

Recently, a breaking change was implemented where the .git folder will not be transmitted to CMP sidecars. @crenshaw-dev suggested making this configurable. This PR sets out to do that via the repo server arguments.

This is my first PR, so apologies if I missed anything!

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 included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • 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.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@codecov
Copy link

codecov bot commented Jun 26, 2022 •
edited

Codecov Report

Merging #9789 (304f1de) into master (a041bf8) will decrease coverage by 0.00%.
The diff coverage is 55.00%.

@@            Coverage Diff             @@
##           master    #9789      +/-   ##
==========================================
- Coverage   45.79%   45.78%   -0.01%     
==========================================
  Files         227      227              
  Lines       26920    26924       +4     
==========================================
+ Hits        12327    12328       +1     
- Misses      12911    12914       +3     
  Partials     1682     1682
Impacted Files Coverage Δ
util/env/env.go 86.95% <0.00%> (-3.96%) ⬇️
util/app/discovery/discovery.go 40.27% <50.00%> (ø)
reposerver/repository/repository.go 63.78% <66.66%> (+0.07%) ⬆️
util/cmp/stream.go 52.23% <100.00%> (-0.36%) ⬇️

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 a041bf8...304f1de. Read the comment docs.

@notfromstatefarm notfromstatefarm marked this pull request as draft June 26, 2022 16:34
@notfromstatefarm
Copy link
Contributor Author

Going to work on this PR a little bit more - need to figure out how to add the environment variables.

@notfromstatefarm notfromstatefarm force-pushed the feat/configurable-tar-filter branch 2 times, most recently from b59bb8f to 57eed89 Compare June 26, 2022 18:26
@notfromstatefarm notfromstatefarm marked this pull request as ready for review June 26, 2022 18:30
@notfromstatefarm
Copy link
Contributor Author

notfromstatefarm commented Jun 26, 2022 •
edited

Figured out how to add the environment variables! It's a little weird compared to the other ones since it's an array. I had to make a way to separate it when using environment variables versus the repeatable flag. Perhaps I should just use the same logic in the flag and make it non-repeatable.

But, all ready and tested locally! I was able to selectively filter out any combination of files and folders.

@notfromstatefarm notfromstatefarm force-pushed the feat/configurable-tar-filter branch 2 times, most recently from 293f080 to 62c0fe5 Compare June 26, 2022 19:02
@notfromstatefarm
Copy link
Contributor Author

Had to fix some small documentation issues for checks to pass. Tested in our real dev clusters. All working. 🎉

crenshaw-dev
crenshaw-dev requested changes Jun 27, 2022
View reviewed changes
cmd/argocd-repo-server/commands/argocd_repo_server.go Outdated Show resolved Hide resolved
docs/operator-manual/argocd-cmd-params-cm.yaml Outdated Show resolved Hide resolved
cmd/argocd-repo-server/commands/argocd_repo_server.go Outdated Show resolved Hide resolved
util/app/discovery/discovery.go Outdated Show resolved Hide resolved
util/cmp/stream_test.go Outdated Show resolved Hide resolved
util/env/env.go Show resolved Hide resolved
@notfromstatefarm
feat: configurable CMP tar exclusions
d4c5ea2 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm
rename cmp to plugin
35ae804 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm
add test for tar stream exclusions
7aa5135 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm
fix tests
3b4460a 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm
merge master
9363086 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm
update cmp guide
ac9b63e 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm
fully parameterize
304f1de 
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@crenshaw-dev crenshaw-dev added the cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch label Jun 27, 2022
@crenshaw-dev crenshaw-dev changed the title feat: configurable CMP tar exclusions (#9675) fix: configurable CMP tar exclusions (#9675) Jun 27, 2022
crenshaw-dev
crenshaw-dev reviewed Jun 27, 2022
View reviewed changes
reposerver/repository/repository.go
@@ -872,7 +873,8 @@ func getRepoCredential(repoCredentials []*v1alpha1.RepoCreds, repoURL string) *v

type GenerateManifestOpt func(*generateManifestOpt)
type generateManifestOpt struct {
cmpTarDoneCh chan<- bool
cmpTarDoneCh chan<- bool
cmpTarExcludedGlobs []string
Copy link
Collaborator

Choose a reason for hiding this comment

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

@leoluz is this type designed to carry the generate params so we can keep that function signature small? If so, 1) @notfromstatefarm went the right direction here and 2) I should probably refactor to put some other things on this struct.

If that's incorrect, we can refactor to pass this param directly to the function.

crenshaw-dev
crenshaw-dev approved these changes Jun 27, 2022
View reviewed changes
Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

@notfromstatefarm lgtm! I have one question for Leo. Once he responds, I expect to be able to merge.

Might be a few days before we get the release out with this fix.

smridge
smridge reviewed Jun 27, 2022
View reviewed changes
USERS.md
@@ -37,6 +37,7 @@ Currently, the following organizations are **officially** using Argo CD:
1. [Chargetrip](https://chargetrip.com)
1. [Chime](https://www.chime.com)
1. [Cisco ET&I](https://eti.cisco.com/)
1. [Cobalt](https://www.cobalt.io/)
Copy link

Choose a reason for hiding this comment

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

❤️

@crenshaw-dev crenshaw-dev merged commit 3550252 into argoproj:master Jun 29, 2022
crenshaw-dev pushed a commit that referenced this pull request Jun 29, 2022
@notfromstatefarm @crenshaw-dev
fix: configurable CMP tar exclusions (#9675) (#9789)
596038f 
* feat: configurable CMP tar exclusions

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>

* rename cmp to plugin

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>

* add test for tar stream exclusions

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>

* fix tests

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>

* update cmp guide

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>

* fully parameterize

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@crenshaw-dev
Copy link
Collaborator

Cherry-picked onto release-2.4 for 2.4.4.

@notfromstatefarm notfromstatefarm mentioned this pull request Jul 4, 2022
Merged
10 tasks
@notfromstatefarm notfromstatefarm mentioned this pull request Aug 9, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smridge smridge smridge left review comments

@crenshaw-dev crenshaw-dev crenshaw-dev approved these changes

@leoluz leoluz Awaiting requested review from leoluz

Assignees
No one assigned
Labels
cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ArgoCD 2.4 introduces breaking change to CMPs not transmitting .git folder
3 participants
@notfromstatefarm @crenshaw-dev @smridge