feat: Matrix generator where a generator can reference items of another one by KojoRising · Pull Request #9080 · argoproj/argo-cd

KojoRising · 2022-04-12T17:54:20Z

Fixes [ISSUE #530]
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:

…ld generators reading parameters. Specifically, 2nd generator reading the 1st generator's parameters. Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

codecov · 2022-04-12T18:08:20Z

Codecov Report

Merging #9080 (598eff0) into master (50bf41f) will increase coverage by 0.00%.
The diff coverage is 60.52%.

@@           Coverage Diff           @@
##           master    #9080   +/-   ##
=======================================
  Coverage   45.82%   45.83%           
=======================================
  Files         222      222           
  Lines       26376    26405   +29     
=======================================
+ Hits        12087    12102   +15     
- Misses      12642    12652   +10     
- Partials     1647     1651    +4

Impacted Files	Coverage Δ
...licationset/generators/generator_spec_processor.go	`51.66% <46.42%> (-5.91%)`	⬇️
...cationset/controllers/applicationset_controller.go	`56.75% <100.00%> (ø)`
applicationset/generators/matrix.go	`72.60% <100.00%> (+0.38%)`	⬆️
applicationset/generators/merge.go	`60.60% <100.00%> (+0.40%)`	⬆️
applicationset/utils/utils.go	`78.40% <100.00%> (ø)`
applicationset/generators/cluster.go	`78.12% <0.00%> (+1.56%)`	⬆️

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 50bf41f...598eff0. Read the comment docs.

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

rishabh625 · 2022-04-13T07:41:30Z

it looks good to me, plz include test case, also it would be great to have the usage documented with example as only second generator refers params of 1st

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising · 2022-04-13T22:00:20Z

@rishabh625 - Awesome sounds great!. Writing up the tests right now. Looking through the ArgoCD Repo as to where I can update edit the Matrix generator docs, but I don't see them under "applicationset" directory, and the regular docs don't seem to mention them. So my question is:

Are the ApplicationSet docs still hosted from the ApplicationSet repository?
If so, should I create a separate PR to add documentation there?

Thanks, and let me know!

rishabh625 · 2022-04-14T12:42:42Z

I guess you need to update docs here, as docs is moved #8955

Are we going to change hosting of application set docs
May be @jgwest @jannfis @crenshaw-dev can answer.

crenshaw-dev · 2022-04-14T17:00:11Z

@KojoRising docs are now kept in docs/operator-manual/applicationset in the argo-cd repo

…strictions regarding child generator B consuming parameters produced by child generator A/ Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising · 2022-04-14T22:36:25Z

Thanks @crenshaw-dev, found it!

I've updated the Matrix Generator documentation to include the following:

An example which shows one child generator consuming parameters from another child generator
Restrictions on the parameter consumption. In particular:

The child generator which consumes parameters must come after the child generator that produces the parameters
Both child generators' cannot consume parameters from one-another (ie. circular dependency issue).

I added the restrictions to the bottom (where the other Matrix Generator Restrictions are), but let me know if I should explicitly make those restrictions clear in the section with the example.

Thanks!

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Lobstrosity · 2022-04-15T23:59:00Z

applicationset/generators/generator_spec_processor.go

+	tmpParams := make(map[string]string)
+	for _, currParam := range params {
+		for k, v := range currParam {
+			tmpParams[k] = v
+		}
+	}


After this block of code, won’t tmpParams contain whatever was in the last map in the params list for a given key? Meaning that the templating below would always interpolate the last value for a given key instead of each value for a given key?

If I’m understanding the use case, it seems params should be a map (instead of a list of maps) and then the matrix generator code would look something like:

g0, err := m.getParams(appSetGenerator.Matrix.Generators[0], appSet, map[string]string{}) for _, a := range g0 { g1, err := m.getParams(appSetGenerator.Matrix.Generators[1], appSet, a) for _, b := range g1 { val, err := utils.CombineStringMaps(a, b) if err != nil { return nil, err } res = append(res, val) } }

…where g1 is determined for each param map a from g0.

So, hmm, so I think basically each "previous" generator's params are stored in each array index. So you are right in that it will only take all of the last generator's params.

But because the Matrix Generator only can have 2 child generators, I think it'll always be an array of size 1. So taking the last index shouldn't be an issue (although know you mentioned it, I think I can possibly replace the entire thing w/ just params[0] instead of tmpParams

So this is how the "params" function parameter value looks like

tmpParams ends up like this:

Also to your point, where the interpolation would only pick up the last value, I also tried it w/ {{path[3]}}, and this is what I got:

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Lobstrosity · 2022-04-18T01:01:12Z

applicationset/generators/generator_spec_processor_test.go

+	}
+	argoCDServiceMock := argoCDServiceMock{mock: &mock.Mock{}}
+
+	argoCDServiceMock.mock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(cases[0].repoApps, cases[0].repoError)


You define several cases but only appear to use the first one.

Ahh yeah, this was more for testing the GetRelevantGenerators function (not mine). I'm fairly new to Go (I'm more on the ArgoCD Yaml side :), so I was using some tests that were used for the Git Generators (where they were specifically testing the Git Generator).

My test cases were below in the TestGetRelevantGenerators function:

Anyways, I cleaned up the above function you mentioned. Thanks for pointing it out!

Lobstrosity · 2022-04-18T01:25:36Z

applicationset/generators/matrix.go

 		return nil, err
 	}
-	g1, err := m.getParams(appSetGenerator.Matrix.Generators[1], appSet)
+	g1, err := m.getParams(appSetGenerator.Matrix.Generators[1], appSet, g0)


Regarding your “I think it'll always be an array of size 1” comment from the other thread, isn’t g0 all items from the first generator? And doesn’t it eventually become the params parameter to your new interpolateGenerator function? I’m unclear how the new example you added to Generators-Matrix.md can be satisfied unless g1 is evaluated for each item in g0, instead of only for the last item in g0 (which we seem to agree is what’s happening based on that loop in interpolateGenerator).

I suggest additional test case(s) in matrix_test.go to validate the complete results from a matrix generator using this functionality (specifically where the first generator yields multiple items).

I’m very interested in this PR for some of my own use cases and I’d try to help out but so far, I’ve been unsuccessful getting a local environment up and running.

Hi @Lobstrosity,

So as to the example you were referring to, I'm not sure if you were able to check my TestInterpolateGenerator function. I had the following example which was very similar to the example I had in Generators-Matrix.md (actually it used more parameters than the example). As you can see in the below test case, the cluster-generator is using 3 different parameters that should be created by the git generator.

If g1 does need to be evaluated for every item in g0, (by item, I'm assuming we're both referring to parameter generated for that particular generator), and that it currently only evaluates the last item, then wouldn't you agree that:

All the keys for this label-selector's values - Be evaluated with the last item in the params list (ie. app3)

All these test-cases be failing?

Also though, I will try your above code snippet and see if I can observe a difference.

g0, err := m.getParams(appSetGenerator.Matrix.Generators[0], appSet, map[string]string{}) for _, a := range g0 { g1, err := m.getParams(appSetGenerator.Matrix.Generators[1], appSet, a) for _, b := range g1 { val, err := utils.CombineStringMaps(a, b) if err != nil { return nil, err } res = append(res, val) } }

Also @KojoRising m unsure the example in Generators-Matrix.md is workable with this code. i would be good to see a e2e test.

…upport' into feat/add-intra-generator-param-support

Lobstrosity · 2022-04-19T22:21:19Z

applicationset/generators/generator_spec_processor_test.go

+	gitGeneratorParams := []map[string]string{
+		{"path": "p1/p2/app3", "path.basename": "app3", "path[0]": "p1", "path[1]": "p2", "path.basenameNormalized": "app3"},
+	}


@KojoRising Regarding this comment and the screenshot with the arrows:

This gitGeneratorParams mock represents a git generator that yields a single result. My concern is, what happens when the git generator yields multiple results? All of those results would be in g0 over in matrix.go but interpolateGenerator is only interpolating values from the first result.

@Lobstrosity

Ahh, got it got it. Yes, you’re right, for multiple paths specified in a git generator, it wouldnt pick those up.

I’ll update the code/test-cases to support this. Thank you for pointing it out!

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising · 2022-04-20T18:04:00Z

@Lobstrosity, @rishabh625, @crenshaw-dev Need some input from your folks here. This issue is in line with what Lobstrosity pointed out above. So roughly

When I replace the templated string, I first get kubernetes.io/app-type: argo-workflows

The issue is that on the 2nd string replacement/interpolation, it searches for the same {{path.basename}} parameter and tries to replace it. But it's already replaced it with argo-workflows, so it doesn't get replaced.

And even if I manage to save a copy of the string, the following is obviously going to be invalid YAML:

==========================================================================================

So my question for y'all is do you think the below:

Should resolve to this:

I can't really think of another way of representing it?. Unless I create two appsets, or two matrix generators with that interpolation resolved.

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

crenshaw-dev · 2022-06-06T16:44:34Z

@KojoRising fixing the DCO check is a pain. Looks like just this one commit message needs to be fixed:

Commit sha: 8427c47, Author: Jay P Kulkarni, Committer: GitHub; Expected "Jay P Kulkarni jkulkarni@ucla.edu", but got "jkulkarn jay.p.kulkarni@blackrock.com".

KojoRising · 2022-06-06T16:47:57Z

Thanks @crenshaw-dev - I was about to ask you for some advice there. I'll try fixing that commit message rn. I was trying these steps from the DCO docs:

Ensure you have a local copy of your branch by checking out the pull request locally via command line.
In your local branch, run: git rebase HEAD~219 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin feat/add-intra-generator-param-support

KojoRising · 2022-06-06T17:22:51Z

@crenshaw-dev any advice here? I was trying to find some resources to help, but I think it just created new commits w/ the changed Author, rather than fixing that commit (I still see DCO identifying that particular commit as an issue).
https://www.git-tower.com/learn/git/faq/change-author-name-email
https://rushlow.dev/blog/oops-i-forgot-to-sign-my-commit-from-last-monday

crenshaw-dev · 2022-06-06T17:50:24Z

I think it's gonna be tedious... basically git rebase -i 2c69828 and change the pick to edit for the two problem commits. https://stackoverflow.com/a/1186549/684776

If you wanna hop on a call, feel free to find me on CNCF Slack! :-)

Changed all numbering back to 1 in "Restrictions" section Signed-off-by: Jay P Kulkarni <jkulkarni@ucla.edu>

1) Removing part of comment in generator_spec_processor.go that was for an earlier iteration of code 2) Returning nil instead of an empty map in matrix.go 3) Creating a full-test (TestInterpolatedMatrixGenerate) in matrix.go. This example is not exactly the same, but very similar, to the example appset I had written in the Matrix Docs (also part of this PR). Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising · 2022-06-06T19:42:59Z

@crenshaw-dev - Can you take a look :))? Think it's good to go now. Thanks again!!

crenshaw-dev

LGTM! Thanks so much all, especially @KojoRising!

KojoRising · 2022-06-06T19:52:05Z

Haha, thanks so much @crenshaw-dev!! Appreciate all the reviewing, it's been a long, but fun PR.

Thanks again @Lobstrosity, @rishabh625, @jannis-a, @rumstead for helping me get through this!!!

michael-bowen-sc · 2022-06-06T20:20:06Z

LGTM! Thanks so much all, especially @KojoRising!

@KojoRising @rumstead amazing stuff gents!

gaeljw · 2022-06-16T11:08:38Z

Thanks everyone for the work. In which version of ArgoCD will this available? 2.4.1?

crenshaw-dev · 2022-06-16T14:10:10Z

@gaeljw since it's a new feature rather than a bugfix, I plan to let it wait until 2.5.0, which should be released in August.

gaeljw · 2022-06-16T15:00:18Z

Sure, good to know @crenshaw-dev 👌

olegeech-me · 2022-06-22T17:42:31Z

Sorry for a dumb question folks, is there a dev image pushed somewhere with this feature inside? @KojoRising @crenshaw-dev
Will k apply -f https://raw.githubusercontent.com/argoproj/applicationset/master/manifests/install.yaml work for me?
I'd like to get use of this feature before it's released in 2.5.

rishabh625 · 2022-06-22T17:48:59Z

applying above manifests won't work @olegeech-me , but I think , applying manifests from this repo master branch should work
kubectl apply -f manifests/install.yaml

KojoRising · 2022-06-22T18:51:01Z

Also @olegeech-me if you wanna try via kustomize (that's how I currently do it): You can just pin it to the latest commit and do a kustomize build [KUSTOMIZE-DIRECTOY] and it should work fine (correct me if I'm wrong @rishabh625)

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: argocd
resources:
- https://github.com/argoproj/argo-cd/manifests/ha/cluster-install?ref=2b5371681f611ca05c1884000bd236a38e02f167

Or non-pinned latest

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: argocd
resources:
- https://github.com/argoproj/argo-cd/manifests/ha/cluster-install

olegeech-me · 2022-06-22T21:35:08Z

thanks a lot! appreciated.

jeromepin · 2022-08-01T07:58:25Z

Is this going to work for merge generators too ?

crenshaw-dev · 2022-08-01T14:32:17Z

@jeromepin I believe this work was limited to only matrix generators, but I could be remembering wrong.

jkulkarn added 3 commits April 12, 2022 10:56

Upgraded Go to 1.18, & have changes related to matrix generator's chi…

d4da216

…ld generators reading parameters. Specifically, 2nd generator reading the 1st generator's parameters. Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Added import for applicationSet in matrix.go

38a2b56

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Added import for applicationSet in matrix.go

5c7ae5a

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising force-pushed the feat/add-intra-generator-param-support branch from 355eaad to 5c7ae5a Compare April 12, 2022 17:56

KojoRising changed the title ~~WIP | Issue #530 | Matrix generator where a generator can reference items of another one~~ feat: Matrix generator where a generator can reference items of another one | Issue #530 | WIP Apr 12, 2022

Added logging + small fixes for ArgoCD PR

f09d41e

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising marked this pull request as ready for review April 12, 2022 21:44

jkulkarn added 3 commits April 13, 2022 12:02

Added testing for GetRelevantGenerators

491d301

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Added a test for interpolating Generators

535a391

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Added a 2nd test for interpolating Generators

4dfa276

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Merge branch 'master' into feat/add-intra-generator-param-support

424f0c9

Updated Generators-Matrix.md documentation to include an example + re…

4edb6fe

…strictions regarding child generator B consuming parameters produced by child generator A/ Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

jkulkarn and others added 2 commits April 14, 2022 18:37

Small wording fix.

ad633e2

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Merge branch 'master' into feat/add-intra-generator-param-support

62c0426

Lobstrosity reviewed Apr 15, 2022

View reviewed changes

Small change to generator_spec_processor.go

a719638

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Lobstrosity reviewed Apr 18, 2022

View reviewed changes

Merge remote-tracking branch 'origin/feat/add-intra-generator-param-s…

78705ce

…upport' into feat/add-intra-generator-param-support

Lobstrosity reviewed Apr 19, 2022

View reviewed changes

Fixing Test case

a6a915b

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

Small changes for matrix + generator_spec_processor.go

6001908

Signed-off-by: jkulkarn <jay.p.kulkarni@blackrock.com>

KojoRising mentioned this pull request Apr 20, 2022

Matrix generator where a generator can reference items of another one argoproj/applicationset#530

Closed

KojoRising force-pushed the feat/add-intra-generator-param-support branch from 0bab357 to 50c70b1 Compare June 6, 2022 16:35

KojoRising force-pushed the feat/add-intra-generator-param-support branch from 62e0ff5 to 4a665ac Compare June 6, 2022 18:18

KojoRising and others added 2 commits June 6, 2022 11:19

Update Generators-Matrix.md

b2d336e

Changed all numbering back to 1 in "Restrictions" section Signed-off-by: Jay P Kulkarni <jkulkarni@ucla.edu>

KojoRising force-pushed the feat/add-intra-generator-param-support branch from 4a665ac to c4c4654 Compare June 6, 2022 18:19

Merge branch 'master' into feat/add-intra-generator-param-support

598eff0

crenshaw-dev approved these changes Jun 6, 2022

View reviewed changes

crenshaw-dev merged commit 001c7b2 into argoproj:master Jun 6, 2022

This was referenced Jul 9, 2022

fix: detect applicationset webhook events for Matrix and Merge genera… #8998

Merged

Applicationset not refreshed after webhook event using matrix generator when using matrix generator references #9931

Closed

jpcosal mentioned this pull request Oct 17, 2022

Merge generator where a generator can reference items of another one argoproj/applicationset#606

Closed

jpcosal mentioned this pull request Oct 25, 2022

Merge generator where a generator can reference items of another one #11054

Open

KojoRising mentioned this pull request Mar 3, 2023

Interpolated Generator can perform redundant parameter generation #12716

Closed

3 tasks

rumstead mentioned this pull request Oct 2, 2024

REQUEST: Promotion to Reviewer for @rumstead argoproj/argoproj#326

Closed

6 tasks

Conversation

KojoRising commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rishabh625 commented Apr 13, 2022

Uh oh!

KojoRising commented Apr 13, 2022

Uh oh!

rishabh625 commented Apr 14, 2022

Uh oh!

crenshaw-dev commented Apr 14, 2022

Uh oh!

KojoRising commented Apr 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KojoRising Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KojoRising commented Apr 20, 2022

Uh oh!

crenshaw-dev commented Jun 6, 2022

Uh oh!

KojoRising commented Jun 6, 2022

Uh oh!

KojoRising commented Jun 6, 2022

Uh oh!

crenshaw-dev commented Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KojoRising commented Jun 6, 2022

Uh oh!

crenshaw-dev left a comment

Choose a reason for hiding this comment

Uh oh!

KojoRising commented Jun 6, 2022

Uh oh!

michael-bowen-sc commented Jun 6, 2022

Uh oh!

gaeljw commented Jun 16, 2022

Uh oh!

crenshaw-dev commented Jun 16, 2022

Uh oh!

gaeljw commented Jun 16, 2022

Uh oh!

olegeech-me commented Jun 22, 2022

Uh oh!

rishabh625 commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KojoRising commented Jun 22, 2022

Uh oh!

olegeech-me commented Jun 22, 2022

Uh oh!

jeromepin commented Aug 1, 2022

Uh oh!

crenshaw-dev commented Aug 1, 2022

KojoRising commented Apr 12, 2022 •

edited

Loading

codecov bot commented Apr 12, 2022 •

edited

Loading

KojoRising Apr 16, 2022 •

edited

Loading

crenshaw-dev commented Jun 6, 2022 •

edited

Loading

rishabh625 commented Jun 22, 2022 •

edited

Loading