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: allow analysis run to use separate kubeconfig for jobs #3350

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

gdsoumya
Copy link
Member

@gdsoumya gdsoumya commented Feb 4, 2024

Fixes #3337

This PR :

  1. Enables setting ARGO_ROLLOUTS_ANALYSIS_JOB_KUBECONFIG to specify custom k8s config for running analysis jobs.
  2. Also provides ARGO_ROLLOUTS_ANALYSIS_JOB_NAMESPACE to specify a different namespace from the analysis run to execute the job.

NOTE:

Setting custom ARGO_ROLLOUTS_ANALYSIS_JOB_NAMESPACE and ARGO_ROLLOUTS_ANALYSIS_JOB_KUBECONFIG will prevent automatic GC of jobs when analysis runs are deleted.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@gdsoumya gdsoumya changed the title feat: allows analysis run to use separate kubeconfig for jobs (wip) feat: allow analysis run to use separate kubeconfig for jobs Feb 4, 2024
Copy link
Contributor

github-actions bot commented Feb 4, 2024

Go Published Test Results

2 130 tests   2 130 ✅  2m 50s ⏱️
  118 suites      0 💤
    1 files        0 ❌

Results for commit 96b2c37.

♻️ This comment has been updated with latest results.

Signed-off-by: Soumya Ghosh Dastidar <[email protected]>
Copy link

codecov bot commented Feb 4, 2024

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (8405f2e) 81.83% compared to head (96b2c37) 82.92%.
Report is 17 commits behind head on master.

Files Patch % Lines
analysis/controller.go 0.00% 14 Missing ⚠️
metricproviders/job/job.go 81.81% 3 Missing and 3 partials ⚠️
pkg/kubectl-argo-rollouts/info/analysisrun_info.go 60.00% 1 Missing and 1 partial ⚠️
utils/controller/controller.go 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3350      +/-   ##
==========================================
+ Coverage   81.83%   82.92%   +1.08%     
==========================================
  Files         135      135              
  Lines       20688    16904    -3784     
==========================================
- Hits        16931    14017    -2914     
+ Misses       2883     2010     -873     
- Partials      874      877       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Feb 4, 2024

E2E Tests Published Test Results

  4 files    4 suites   3h 39m 13s ⏱️
107 tests  97 ✅  6 💤 4 ❌
434 runs  404 ✅ 24 💤 6 ❌

For more details on these failures, see this check.

Results for commit 96b2c37.

♻️ This comment has been updated with latest results.

@gdsoumya gdsoumya marked this pull request as draft February 5, 2024 05:50
@gdsoumya gdsoumya changed the title (wip) feat: allow analysis run to use separate kubeconfig for jobs feat: allow analysis run to use separate kubeconfig for jobs Feb 6, 2024
@gdsoumya gdsoumya marked this pull request as ready for review February 6, 2024 08:21
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

I did a careful review of this one and could not find anything. This LGTM!

@jessesuen
Copy link
Member

@zachaller did you want a chance to review this before merging? FWIW, we have also done manual, end-to-end testing of the feature.

@zachaller
Copy link
Collaborator

zachaller commented Feb 16, 2024

I will take deeper look tomorrow, however one of my first thoughts is consistency with ENV var naming. Due to past conventions both plugins and various metric providers use ARGO_ROLLOUTS_ prefixes for env vars. I lean towards keeping consistent with that even though this somewhat piggy backs off of kubeclients KUBECONFIG env var. I tend to always prefer consistency though. Thoughts?

@gdsoumya
Copy link
Member Author

gdsoumya commented Feb 16, 2024

I will take deeper look tomorrow, however one of my first thoughts is consistency with ENV var naming. Due to past conventions both plugins and various metric providers use ARGO_ROLLOUTS_ prefixes for env vars. I lean towards keeping consistent with that even though this somewhat piggy backs off of kubeclients KUBECONFIG env var. I tend to always prefer consistency though. Thoughts?

Would you prefer ARGO_ROLLOUT_ANALYSIS_JOB_KUBECONFIG and ARGO_ROLLOUTS_ANALYSIS_JOB_NAMESPACE ? I dropped the prefix as the env was getting a bit long and also because we recently added the only analysis mode which makes it a standalone feature so thought just having ANALYSIS_ prefix might work. Though I don't have a strong opinion on this and if you and @jessesuen feel a bigger env name isn't an issue I'll add it.

@jessesuen
Copy link
Member

Thanks @zachaller, that's a great call out. I agree the ARGO_ROLLOUTS_ prefix is an important convention to keep. So despite the long name, let's use that.

Signed-off-by: Soumya Ghosh Dastidar <[email protected]>
@gdsoumya
Copy link
Member Author

Updated env names

Copy link

sonarcloud bot commented Feb 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@zachaller zachaller left a comment

Choose a reason for hiding this comment

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

LGTM

@zachaller zachaller added this to the v1.7 milestone Feb 27, 2024
@zachaller zachaller merged commit dc6da7d into argoproj:master Feb 28, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to specify a different KUBECONFIG where Analysis vs. Jobs would run
3 participants