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: server-side manifest generation for local diff (#8145) #10019

Merged
merged 26 commits into from Aug 17, 2022

Conversation

notfromstatefarm
Copy link
Contributor

@notfromstatefarm notfromstatefarm commented Jul 17, 2022

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

Closes #8145

This implements diffs of local manifests without having config management tools or CMPs locally installed, by streaming manifests from the CLI to the repo-server for manifest generation.

Adds:

  • --server-side-generate parameter to argocd app diff
  • Maximum tar size and extracted file size, configurable via repo server params. This is to prevent someone overwhelming the repo server. By default 100mb for tar size, 1024mb for extracted size.
  • Only sends yaml/json files by default, configurable with --local-include

It also refactors a little bit of the CMP stream for DRYness since that stream and this stream shares functionality.

Demo showing server-side diff and local diff being identical:
image

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm notfromstatefarm changed the title feat: server-side manifest generation for diff (#8145) feat: server-side manifest generation for local diff (#8145) Jul 17, 2022
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm notfromstatefarm marked this pull request as draft July 18, 2022 13:35
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #10019 (a1d488d) into master (68d0ef0) will decrease coverage by 0.15%.
The diff coverage is 30.49%.

@@            Coverage Diff             @@
##           master   #10019      +/-   ##
==========================================
- Coverage   46.25%   46.09%   -0.16%     
==========================================
  Files         228      229       +1     
  Lines       27883    28140     +257     
==========================================
+ Hits        12897    12972      +75     
- Misses      13247    13404     +157     
- Partials     1739     1764      +25     
Impacted Files Coverage Δ
cmd/argocd/commands/app.go 19.25% <0.00%> (-0.27%) ⬇️
reposerver/repository/repository.go 61.81% <0.00%> (-2.60%) ⬇️
server/application/application.go 30.28% <1.78%> (-1.58%) ⬇️
util/cmp/stream.go 54.95% <42.85%> (+2.71%) ⬆️
util/io/files/tar.go 52.99% <48.14%> (-3.58%) ⬇️
util/manifeststream/stream.go 55.88% <55.88%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
@notfromstatefarm notfromstatefarm marked this pull request as ready for review July 19, 2022 02:10
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
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.

Nitpicky stuff. This looks awesome!

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
cmd/argocd/commands/app.go Show resolved Hide resolved
reposerver/repository/repository.go Show resolved Hide resolved
reposerver/repository/repository.go Show resolved Hide resolved
cmd/argocd/commands/app.go Outdated Show resolved Hide resolved
util/manifeststream/stream.go Outdated Show resolved Hide resolved
util/manifeststream/stream.go Show resolved Hide resolved
util/manifeststream/stream.go Outdated Show resolved Hide resolved
util/tgzstream/stream.go Show resolved Hide resolved
…arning

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

@notfromstatefarm can you resolve that conflict?

@crenshaw-dev
Copy link
Collaborator

@notfromstatefarm apologies for the slow review, can you resolve conflicts again? :-)

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

notfromstatefarm commented Aug 17, 2022

@crenshaw-dev I thiiiiink that's a flake in Integration tests / Run unit tests for Go packages (pull_request)? can you rerun?

Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
Signed-off-by: notfromstatefarm <86763948+notfromstatefarm@users.noreply.github.com>
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.

lgtm! I think we need a follow-up issue because --local . is going to cause the RelativePath() check to fail. But otherwise, local testing checks out.

@crenshaw-dev crenshaw-dev merged commit 95987d8 into argoproj:master Aug 17, 2022
@MeNsaaH
Copy link
Contributor

MeNsaaH commented Sep 16, 2022

@crenshaw-dev @notfromstatefarm please, what release is this in? I've installed the latest release and can't find the changes in this yet

@crenshaw-dev
Copy link
Collaborator

@MeNsaaH it will be in 2.5 (RC1 coming in the next 2 weeks).

@MeNsaaH
Copy link
Contributor

MeNsaaH commented Sep 16, 2022

Okay. Thank you @crenshaw-dev .

@myrondev
Copy link

myrondev commented Nov 15, 2022

Hi. Is this issue: agocd app diff failing with the URL scheme 'secrets' is not allowed still hasn't been solved in any argocd release?

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.

argocd app sync/diff --local doesn't account for sidecar CMPs
4 participants