feat!: remove rbac when using argocd cluster rm (#8958)

[danielhelfand]

Closes #8958

This change will remove rbac used by Argo CD for creating resources on external clusters once the cluster is removed.

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:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Merging #8969 (b32deab) into master (89c9c62) will decrease coverage by 0.03%.
The diff coverage is 34.61%.

@@            Coverage Diff             @@
##           master    #8969      +/-   ##
==========================================
- Coverage   45.22%   45.19%   -0.04%     
==========================================
  Files         214      214              
  Lines       25424    25438      +14     
==========================================
- Hits        11499    11497       -2     
- Misses      12307    12324      +17     
+ Partials     1618     1617       -1     

Impacted Files	Coverage Δ
cmd/argocd/commands/cluster.go	9.17% <34.61%> (+3.58%)	⬆️
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 89c9c62...b32deab. Read the comment docs.

[crenshaw-dev]

Can we get some test coverage on this? I'm not sure how difficult that might be, since it involves multiple clusters.

Also, should we consider this a breaking change? Seems unlikely, but I could imagine a script which assumes the config will be removed but the RBAC will remain.

[danielhelfand]

Can we get some test coverage on this? I'm not sure how difficult that might be, since it involves multiple clusters.

I'll look into some of the e2e tests and see how challenging this would be. But yeah, that was my only initial hesitation was how complex the test could end up being.

Also, should we consider this a breaking change? Seems unlikely, but I could imagine a script which assumes the config will be removed but the RBAC will remain.

Yeah, maybe scripts exist to clean up the rbac after the fact. I would consider it breaking in that sense. But hopefully that's more of an edge case, and I would assume this would be a welcome change by anyone doing that.

If we want to treat this as breaking, fine by me. Just let me know what that process would be to introduce a change like this then.

[crenshaw-dev]

If e2e tests are too complex, maybe we can just test getRestConfig.

@alexmt what's the process for a minor breaking change? I'm thinking:

1. add an exclamation mark to the issue name so we remember to make a note in release notes
2. add a breaking label to this PR so we avoid cherry-picking it unless someone really wants this feature

[danielhelfand]

@crenshaw-dev Added some unit tests for getRestConfig and an e2e test for ClusterDeletion/RBAC is removed. I'll keep an eye out for updates on introducing this as a breaking change.

[crenshaw-dev]

I'm just gonna roll with the label and the title change. Someone can correct me later if that's wrong. Will review the additional tests!

[crenshaw-dev]

LGTM!