feat: Make spec.source.helm.values field accepts either a string or a rawExtension #8794

mcbenjemaa · 2022-03-16T16:03:13Z

Test using this demo repo: https://github.com/mcbenjemaa/argocd-apps

Fixes: #2936
Enhancement Proposal Ref: #8579

Thanks @crenshaw-dev for guidance.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

mcbenjemaa · 2022-03-16T17:55:06Z

I will fix the tests

mcbenjemaa · 2022-03-16T17:56:14Z

Fixes: #2936

crenshaw-dev · 2022-03-17T18:56:55Z

@mcbenjemaa thanks for your persistence on this! I want to make sure I have enough time to give this a solid review, so bear with me as I try to carve out the time.

crenshaw-dev

Just tested this locally, and things seem to behave quite nicely! We could probably use an e2e test or two.

My questions might have been silly. Mostly stuff that surprised me / I was curious about, not necessarily anything wrong!

pkg/apis/application/v1alpha1/stringorobject_types.go

ui/src/app/applications/components/application-parameters/application-parameters.tsx

codecov · 2022-03-22T14:54:13Z

Codecov Report

Merging #8794 (f201dc1) into master (ba0c249) will decrease coverage by 0.13%.
The diff coverage is 7.93%.

@@            Coverage Diff             @@
##           master    #8794      +/-   ##
==========================================
- Coverage   44.96%   44.82%   -0.14%     
==========================================
  Files         212      213       +1     
  Lines       25261    25318      +57     
==========================================
- Hits        11359    11350       -9     
- Misses      12295    12363      +68     
+ Partials     1607     1605       -2

Impacted Files	Coverage Δ
cmd/argocd/commands/app.go	`9.13% <0.00%> (-0.04%)`	⬇️
.../apis/application/v1alpha1/stringorobject_types.go	`2.77% <2.77%> (ø)`
cmd/util/app.go	`48.70% <6.25%> (-1.70%)`	⬇️
pkg/apis/application/v1alpha1/types.go	`54.63% <100.00%> (ø)`
reposerver/repository/repository.go	`60.08% <100.00%> (ø)`
applicationset/services/scm_provider/github.go	`63.52% <0.00%> (-17.65%)`	⬇️
applicationset/services/scm_provider/utils.go	`88.50% <0.00%> (+4.59%)`	⬆️

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 ba0c249...f201dc1. Read the comment docs.

cmd/util/app.go

mcbenjemaa · 2022-03-25T14:17:14Z

Strange! Why this test is failing docs/readthedocs.org:argo-cd

crenshaw-dev · 2022-03-25T14:23:37Z

@mcbenjemaa oh, there was a dependency in docs/requirements.txt that needed to be pinned. If you merge master, it should work. :-)

crenshaw-dev · 2022-03-29T13:16:30Z

Hey, @mcbenjemaa, how's this going? Anything I can do to help?

mcbenjemaa · 2022-03-29T15:59:27Z

I have rebased it with master!
If the tests succeeds we are ready!

crenshaw-dev · 2022-03-29T16:56:46Z

@mcbenjemaa could you run make codegen-local? Looks like the CLI docs need to be updated.

mcbenjemaa · 2022-03-31T07:43:32Z

I don't know Why

license/snyk (Argoproj) test has failed
security/snyk (Argoproj) test has failed

mcbenjemaa · 2022-03-31T10:21:33Z

@crenshaw-dev Can you restart the test ?

Integration tests / Run end-to-end tests (v1.22.6)

crenshaw-dev · 2022-03-31T13:48:15Z

I don't think I have permissions to rerun tests. @alexmt can you kick it off again?

Alternatively @mcbenjemaa, you could push an empty commit.

mcbenjemaa · 2022-03-31T15:40:33Z

Just rebasing it.

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

…g or object Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Signed-off-by: Mohamed Chiheb <mohamed.chiheb-extern@renault.com>

…e2e that sets values as raw via this flag Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com> Signed-off-by: Mohamed Chiheb <mohamed.chiheb-extern@renault.com> Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

crenshaw-dev

Alrighty. I'm super excited about this PR. It's been a marathon, thanks for all your work @mcbenjemaa! I want to do a line-by-line review of this PR and do some manual testing. So please be patient with any comments that come up. But this is really good code, I'm excited to get it merged.

assets/swagger.json

crenshaw-dev · 2022-03-31T17:28:35Z

cmd/argocd/commands/app.go

@@ -632,7 +633,7 @@ func NewApplicationUnsetCommand(clientOpts *argocdclient.ClientOptions) *cobra.C
 				}
 			}
 			if app.Spec.Source.Helm != nil {
-				if len(parameters) == 0 && len(valuesFiles) == 0 && !valuesLiteral && !ignoreMissingValueFiles {
+				if len(parameters) == 0 && len(valuesFiles) == 0 && !valuesLiteral && !valuesRawLiteral && !ignoreMissingValueFiles {


I'd like to see a CLI unit test for this param. Recently merged tests like #8918 and #8901 might be helpful.

cmd/util/app.go

crenshaw-dev · 2022-03-31T17:32:34Z

cmd/util/app.go

 	if len(opts.values) > 0 {
-		src.Helm.Values = opts.values
+		src.Helm.Values.Values = opts.values
+	}
+	if len(opts.valuesRaw) > 0 {


What happens if someone specifies both? Should we throw an error if they try?

pkg/apis/application/v1alpha1/stringorobject_types.go

crenshaw-dev · 2022-04-06T14:03:33Z

@mcbenjemaa lmk if I can do anything to help with this!

crenshaw-dev · 2022-04-12T17:21:57Z

@mcbenjemaa closing this in favor of #9079, where I'll try to get it across the finish line. Thanks so much again for all your work on this!

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch 3 times, most recently from 0ab763c to a8d18af Compare March 16, 2022 16:10

crenshaw-dev reviewed Mar 17, 2022

View reviewed changes

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch 7 times, most recently from dd329e5 to 6e4f706 Compare March 22, 2022 13:48

mcbenjemaa commented Mar 23, 2022

View reviewed changes

cmd/util/app.go Show resolved Hide resolved

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch 7 times, most recently from 78a2375 to 9ad1808 Compare March 25, 2022 14:16

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch from 9ad1808 to 22734f7 Compare March 29, 2022 15:57

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch from 22734f7 to e1756fb Compare March 30, 2022 15:51

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch from e1756fb to 5f5ba9d Compare March 30, 2022 16:43

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch from 5f5ba9d to bd90ede Compare March 31, 2022 07:44

mcbenjemaa and others added 10 commits March 31, 2022 16:40

Make values accepts a string and an object

2dda807

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Fix string Values

4abd841

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Make argocd application spec.source.hem.values accepts either a strin…

61ca96d

…g or object Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Add org to users

b5f6ecd

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Fix e2e test and fix lint

e23b384

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Add unit test for helm values as raw format

953ce0a

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Fix MarshalJSON

831035a

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

Fix TestHelmValuesLiteralFileRemote test

40533b2

Signed-off-by: Mohamed Chiheb <mohamed.chiheb-extern@renault.com>

Regenrate

f201dc1

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>

mcbenjemaa force-pushed the feat/argocd-helm-values-raw branch from bd90ede to f201dc1 Compare March 31, 2022 15:41

crenshaw-dev reviewed Mar 31, 2022

View reviewed changes

crenshaw-dev requested changes Apr 4, 2022

View reviewed changes

crenshaw-dev mentioned this pull request Apr 12, 2022

feat: Make spec.source.helm.values field accepts either a string or a rawExtension #9079

Closed

crenshaw-dev closed this Apr 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make spec.source.helm.values field accepts either a string or a rawExtension #8794

feat: Make spec.source.helm.values field accepts either a string or a rawExtension #8794

mcbenjemaa commented Mar 16, 2022 •

edited by crenshaw-dev

Loading

mcbenjemaa commented Mar 16, 2022

mcbenjemaa commented Mar 16, 2022

crenshaw-dev commented Mar 17, 2022

crenshaw-dev left a comment

codecov bot commented Mar 22, 2022 •

edited

Loading

mcbenjemaa commented Mar 25, 2022

crenshaw-dev commented Mar 25, 2022

crenshaw-dev commented Mar 29, 2022

mcbenjemaa commented Mar 29, 2022

crenshaw-dev commented Mar 29, 2022

mcbenjemaa commented Mar 31, 2022

mcbenjemaa commented Mar 31, 2022

crenshaw-dev commented Mar 31, 2022

mcbenjemaa commented Mar 31, 2022 •

edited

Loading

crenshaw-dev left a comment

crenshaw-dev Mar 31, 2022

crenshaw-dev Mar 31, 2022

crenshaw-dev commented Apr 6, 2022

crenshaw-dev commented Apr 12, 2022

feat: Make spec.source.helm.values field accepts either a string or a rawExtension #8794

feat: Make spec.source.helm.values field accepts either a string or a rawExtension #8794

Conversation

mcbenjemaa commented Mar 16, 2022 • edited by crenshaw-dev Loading

mcbenjemaa commented Mar 16, 2022

mcbenjemaa commented Mar 16, 2022

crenshaw-dev commented Mar 17, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2022 • edited Loading

Codecov Report

mcbenjemaa commented Mar 25, 2022

crenshaw-dev commented Mar 25, 2022

crenshaw-dev commented Mar 29, 2022

mcbenjemaa commented Mar 29, 2022

crenshaw-dev commented Mar 29, 2022

mcbenjemaa commented Mar 31, 2022

mcbenjemaa commented Mar 31, 2022

crenshaw-dev commented Mar 31, 2022

mcbenjemaa commented Mar 31, 2022 • edited Loading

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev Mar 31, 2022

Choose a reason for hiding this comment

crenshaw-dev Mar 31, 2022

Choose a reason for hiding this comment

crenshaw-dev commented Apr 6, 2022

crenshaw-dev commented Apr 12, 2022

mcbenjemaa commented Mar 16, 2022 •

edited by crenshaw-dev

Loading

codecov bot commented Mar 22, 2022 •

edited

Loading

mcbenjemaa commented Mar 31, 2022 •

edited

Loading