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(api): add endpoints to get AnalysisTemplate config resources #2480

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

devholic
Copy link
Member

This commit adds endpoints to get env resources - ConfigMap and Secret - which can be used in the AnalysisTemplate, to allow users to manage those resources in the Kargo UI. The ConfigMap and Secret must be labeled with kargo.akuity.io/analysis-env: "true" to be returned in API.

Note

  • We may leverage existing Create/Update/DeleteResource API to manage ConfigMap and Secret, same as the AnalysisTemplate (cc @gdsoumya)

Open Questions

  • I'm not sure about the kargo.akuity.io/analysis-env key naming, could anyone give other suggestions?

This commit adds endpoints to get env resources - ConfigMap and Secret
- which can be used in the AnalysisTemplate, to allow users to manage
those resources in the Kargo UI.

Signed-off-by: Sunghoon Kang <hoon@akuity.io>
Copy link

netlify bot commented Aug 29, 2024

Deploy Preview for docs-kargo-akuity-io canceled.

Name Link
🔨 Latest commit a3a8feb
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66e10a2157f096000856b482

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 160 lines in your changes missing coverage. Please review.

Project coverage is 48.32%. Comparing base (2ab4270) to head (a3a8feb).
Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
...l/api/get_analysis_template_config_map_v1alpha1.go 0.00% 52 Missing ⚠️
...ernal/api/get_analysis_template_secret_v1alpha1.go 0.00% 52 Missing ⚠️
...api/list_analysis_template_config_maps_v1alpha1.go 0.00% 28 Missing ⚠️
...nal/api/list_analysis_template_secrets_v1alpha1.go 0.00% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2480      +/-   ##
==========================================
+ Coverage   48.27%   48.32%   +0.04%     
==========================================
  Files         246      254       +8     
  Lines       17739    18133     +394     
==========================================
+ Hits         8564     8762     +198     
- Misses       8749     8889     +140     
- Partials      426      482      +56     

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

Copy link
Contributor

@gdsoumya gdsoumya left a comment

Choose a reason for hiding this comment

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

LGTM

@hiddeco
Copy link
Contributor

hiddeco commented Aug 30, 2024

What about just calling the annotation /ui-managed (to compliment /managed), so that we can eventually utilize it in other places without having to introduce a bazillion annotations?

@hiddeco
Copy link
Contributor

hiddeco commented Aug 30, 2024

The other option I can think of, as it's theoretically not just the UI, is to call it /api-managed or /api-accessible.

@hiddeco
Copy link
Contributor

hiddeco commented Aug 30, 2024

Talked this through some more offline, and the worry with a /api-managed or /api-accessible endpoint is that a value of true would not allow the caller to "categorize" the ConfigMaps and/or Secrets they retrieve.

Given this, and with an eye on the future, I propose looking for a generic label name with a special categorized value, e.g., env. This way, we allow room for future enhancements in which we add new categories, potentially using a generic endpoint that accepts a query parameter to set the value.

@krancour krancour added this to the v0.10.0 milestone Aug 31, 2024
#2480 (comment)

Signed-off-by: Sunghoon Kang <hoon@akuity.io>
@devholic
Copy link
Member Author

devholic commented Sep 2, 2024

@hiddeco

Talked this through some more offline, and the worry with a /api-managed or /api-accessible endpoint is that a value of true would not allow the caller to "categorize" the ConfigMaps and/or Secrets they retrieve.

Given this, and with an eye on the future, I propose looking for a generic label name with a special categorized value, e.g., env. This way, we allow room for future enhancements in which we add new categories, potentially using a generic endpoint that accepts a query parameter to set the value.

If we have generic label name and using categorized value, couldn’t we leverage the Kubernetes’s label selector? For example, kargo.akuity.io/configmaps: "analysis-env" and if we add another categorized value (such as) stage-env , the label should be kargo.akuity.io/configmaps: "analysis-env, stage-env" but not sure if we can leverage label selector in this case.

@hiddeco
Copy link
Contributor

hiddeco commented Sep 2, 2024

Yes, you can use a label selector, which we have already been doing for credential secrets (and what is being practiced in this PR). However, this does not work when getting a specific object, which requires additional verification.

@devholic
Copy link
Member Author

devholic commented Sep 3, 2024

Then I'll choose to use kargo.akuity.io/analysis-run-template: "config" for both ConfigMap and Secret - The reason I chose config over env is that both of them can be used as a volume

WDYT? @hiddeco @gdsoumya

@devholic devholic changed the title feat(api): add endpoints to get AnalysisTemplate env resources feat(api): add endpoints to get AnalysisTemplate config resources Sep 4, 2024
@krancour krancour modified the milestones: v0.10.0, v0.9.0 Sep 10, 2024
@devholic devholic added this pull request to the merge queue Sep 11, 2024
@devholic devholic removed this pull request from the merge queue due to a manual request Sep 11, 2024
#2480 (comment)

Signed-off-by: Sunghoon Kang <hoon@akuity.io>
@devholic devholic added this pull request to the merge queue Sep 11, 2024
Merged via the queue into main with commit 3d705c1 Sep 11, 2024
16 of 17 checks passed
@devholic devholic deleted the hoon/analysis-run-resources-api branch September 11, 2024 03:33
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.

4 participants