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

adds a default clearml-core service account for the core components #253

Merged
merged 8 commits into from
Sep 27, 2023

Conversation

thoraxe
Copy link
Contributor

@thoraxe thoraxe commented Aug 24, 2023

What this PR does / why we need it:
This PR adds a default service account declaration to the clearml components (webserver, fileserver, apiserver) and a YAML to the chart to create that SA.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Verify the work you plan to merge addresses an existing issue (If not, open a new one) (required)
  • Check your branch with helm lint (required)
  • Update version in Chart.yaml according semver rules (required)
  • Substitute annotations section in Chart.yaml annotating implementations (useful for Artifecthub changelog) (required)
  • Update chart README using helm-docs (required)

Which issue(s) this PR fixes:
Fixes #247

Special notes for your reviewer:
It's not entirely a complete PR because it should either do one of two things, which I'm not entirely sure how to do:

  1. enable or disable using the ServiceAccount at all
  2. create individual SAs for each component based on the name specified in values.yaml

Copy link
Contributor

@valeriano-manassero valeriano-manassero left a comment

Choose a reason for hiding this comment

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

README needs to be synced

charts/clearml/templates/serviceAccount.yaml Outdated Show resolved Hide resolved
@valeriano-manassero
Copy link
Contributor

@thoraxe I did some changes to reach CI ok status.
Would you mind to check just to be sure this is what you needed in this PR?

@thoraxe
Copy link
Contributor Author

thoraxe commented Sep 26, 2023

It looks like it does the job to me -- it adds a default service account for the core components with a value. Thanks for cleaning up my mess!

@valeriano-manassero valeriano-manassero merged commit a1673ae into allegroai:main Sep 27, 2023
10 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.

[allegroai/clearml] clearml apiserver needs to run as root but deployment doesn't specify
2 participants