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

N:1 left join for Merge generator #11952

Open
sergej-laufer opened this issue Jan 11, 2023 · 5 comments · May be fixed by #16080
Open

N:1 left join for Merge generator #11952

sergej-laufer opened this issue Jan 11, 2023 · 5 comments · May be fixed by #16080
Labels
component:application-sets Bulk application management related enhancement New feature or request

Comments

@sergej-laufer
Copy link

sergej-laufer commented Jan 11, 2023

I was expecting the merge generator to work like a SQL left join. But it does not allow a N:1 mapping, where the base has a non unique key.

In the following example there are multiple clusters with dev, stg and prod env label. I would expect that all clusters would merge with the list generator, that has only one matching entry for each env. Instead I get the error the parameters from a generator were not unique by the given mergeKeys, Merge requires all param sets to be unique.

  generators:
    - merge:
        mergeKeys: [metadata.labels.env]
        generators:
          - clusters: {}
          - list: {elements: [{metadata.labels.env: dev, values.gitref: 6dba68d59f8679479ba1690336d8d8775c6057ac },
                              {metadata.labels.env: stg, values.gitref: DEVOPS-12000-argocd},
                              {metadata.labels.env: prod, values.gitref: main}]}

A work around is to combine a matrix and merge generator.

  generators:
    - matrix:
        generators:
          - clusters: {}
          - merge:
              mergeKeys: [env]
              generators:
                - list: {elements: [{env: '{{ "{{metadata.labels.env}}" }}' }]}
                - list: {elements: [{env: dev, values.gitref: 4b514e8e9f9942c296e21e448da5691c5b4f4310 },
                                    {env: stg, values.gitref: DEVOPS-12000-argocd},
                                    {env: prod, values.gitref: main}]}

I dont see a reason to forbid non unique keys on the base generator. Allowing non unique key for the base generator would make the Merge generator more useful and would simplify the configuration for usecases like in the example.

@sergej-laufer sergej-laufer added the enhancement New feature or request label Jan 11, 2023
@crenshaw-dev
Copy link
Member

I think other merge "modes" would be really useful.

How would you feel about something like this?

generators:
- merge:
  mode: left-join

@sergej-laufer
Copy link
Author

I like that idea. Would be great to add more modes in the future (e.g. inner-join , left-outer-join ...)

Wondering what the current merge mode should be called. It works like a left-join that strictly only allows unique keys.

@crenshaw-dev
Copy link
Member

Good question. left-unique?

When I wrote the original merge generator code, I was writing for a specific internal use case with the expectation that other modes would eventually be added. I tried to use a SQL analogy for our use case, but it quickly broke down. :-)

Whatever name makes it most clear for the user is I think best.

@ishitasequeira ishitasequeira added the component:application-sets Bulk application management related label Jan 12, 2023
@defyjoy
Copy link

defyjoy commented Feb 7, 2023

While this is a really a very unique case but this is exactly what I am stuck with right now .

@scrocquesel
Copy link
Contributor

inner-join would help in not generating application without secondary generator param values.
Actually, when a base param set is not matched in the secondary param sets, if you use a param from the secondary generator in the template, it will be replaced by <no value> which is definitely wrong.

Maybe merge should be left as-is and have a join plugin ? I mean merge allows to override param of previous generator while join would not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:application-sets Bulk application management related enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants