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(argo-cd): represent cluster credentials as a map #2648

Merged
merged 1 commit into from
May 28, 2024

Conversation

nbarrientos
Copy link
Contributor

@nbarrientos nbarrientos commented Apr 19, 2024

Closes #2592.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

I'll re-generate the docs and bump the version if this change request goes forward. This contains non-backwards compatible changes. The rationale for this change is in the issue it closes.

Thanks for considering it.

@nbarrientos nbarrientos changed the title Represent cluster credentials as a map feat(argo-cd): represent cluster credentials as a map Apr 19, 2024
@nbarrientos
Copy link
Contributor Author

Similar breaking change recently accepted: 237493a

#2523

@nbarrientos nbarrientos force-pushed the cern_upstream branch 4 times, most recently from 7101b95 to cdde9ed Compare April 29, 2024 13:20
@jgranieczny
Copy link

Hi, any news on this? I would be very useful for us.

@pdrastil
Copy link
Member

Hi, I'm for getting this in main. As this is breaking change for lots of people I would personally bundle this with another breaking things in next major release or do it as 2 phase process where old / new implementation lives side by side and old one will be removed later.

@nbarrientos
Copy link
Contributor Author

Thanks! Fine for me to bundle with more breaking changes in the future; we're okay with keeping a local fork in the meantime.

@mkilchhofer
Copy link
Member

Hi, I'm for getting this in main. As this is breaking change for lots of people I would personally bundle this with another breaking things in next major release or do it as 2 phase process where old / new implementation lives side by side and old one will be removed later.

We can also create multiple breaking change, IMHO we do not need to wait. But I am fine with both :)

@nbarrientos
Copy link
Contributor Author

Wondering if we could give this a push this week? :)

@mbevc1
Copy link
Collaborator

mbevc1 commented May 27, 2024

There are some conflicts that needs resolving as well

@nbarrientos
Copy link
Contributor Author

Yes, no worries, I'll fix that once somebody gives me the green light. I'd rather to avoid having to fix them several times.

@mkilchhofer
Copy link
Member

🟢 from my side 👍

@mbevc1
Copy link
Collaborator

mbevc1 commented May 27, 2024

Same ✔️

@nbarrientos
Copy link
Contributor Author

Conflicts resolved, version bumped to 7.0.0 due to the backwards-incompatible change.

Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

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

Line 10 and 14 also requires changes

  • {{- with .labels }}
  • {{- with .annotations }}

@nbarrientos
Copy link
Contributor Author

nbarrientos commented May 28, 2024

Line 10 and 14 also requires changes

* `{{- with .labels }}`

* `{{- with .annotations }}`

Thanks for the review.

I've added the requested changes, however it seems to work as expected (Helm v3.14.3) without specifying the scope in the with block. Perhaps inside the range, within the with block the context expands to all the local variables (in that case $cluster_key and $cluster_value).

With these values:

configs:
  clusterCredentials:
    mycluster2:
      server: https://mycluster2.example.com
      labels:
        foo: bar
      annotations:
        bar: foo
      namespaces: namespace1,namespace2
      clusterResources: true
      config:
        bearerToken: "<authentication token>"
        tlsClientConfig:
          insecure: false
          caData: "<base64 encoded certificate>"

The Secret object renders fine (both the labels and the annotations are included) with and without the modifications.

So it can be merged with values coming from different sources.

Closes argoproj#2592

Signed-off-by: Nacho Barrientos <nacho.barrientos@cern.ch>
Copy link
Member

@mkilchhofer mkilchhofer left a comment

Choose a reason for hiding this comment

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

Thanks a lot @nbarrientos 🙏

@mbevc1 mbevc1 merged commit 2c05baf into argoproj:main May 28, 2024
6 checks passed
@renovate renovate bot mentioned this pull request Oct 4, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change clusterCredentials type to map
6 participants