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

Helm chart: Own service accounts for API and Web #621

Merged

Conversation

johnksv
Copy link
Contributor

@johnksv johnksv commented Oct 10, 2023

See issue #616 which this PR resolves.

Question: Where should i update the doc (as this is a breaking change)?

PS: I have not had the opportunity to test this yet, but wanted to create the PR such that it can be discussed.

Commit message describing the change:

Service account was previously specified at root level. When using serviceAccount.create, this was OK, since api and web would then get their own SAs. When serviceAccount.create was false however, the same SA would be used for both api and web, which is very problematic of several reasons.

This commit fix this issue by moving the serviceAccount field from root to both api and web. This also make it clearer what happen under-the-hood.
The change is breaking, and major version is therefore bumped.

@olevitt olevitt self-requested a review October 12, 2023 12:35
@olevitt
Copy link
Contributor

olevitt commented Oct 12, 2023

LGTM (tested on our setup) except the fact that I think you should not bump the helm chart version manually as it's handled by the CI but maybe @garronej can enlighten us on this ?
@johnksv could you enable "allow commits from maintainers" from your PR so that we can adjust it (e.g rebase / remove the helm version bump) before merging ?

Service account was previously specified at root level. When using serviceAccount.create, this was OK, since api and web would then get their own SAs. When serviceAccount.create was false however, the same SA would be used for both api and web, which is very problematic of several reasons.

This commit fix this issue by moving the serviceAccount field from root to both api and web. This also make it clearer what happen under-the-hood.
The change is breaking, and major version is therefore bumped.
@johnksv johnksv force-pushed the helm-chart-service-account-refactor branch from 0381302 to d4bc723 Compare October 12, 2023 15:59
@johnksv
Copy link
Contributor Author

johnksv commented Oct 12, 2023

Thanks, @olevitt . Rebased on main now and reverted bump of chart version.
Looks like "allow commits from maintainers" is not an option. Hm, maybe that is because of some settings on the fork.

@garronej
Copy link
Contributor

garronej commented Oct 12, 2023

@johnksv Thanks a lot for this PR!

I think you should not bump the helm chart version manually as it's handled by the CI but maybe @garronej can enlighten us on this ?

@olevitt @johnksv, You did good by bumping the Helm chart version to the next major. The deployment workflow automatically bump the chart when there is a bump on either -web or -api but you can also pub the helm chart version manually, this too will triger a release.
Beside, our workflow isn't smart to the point of being able to figure out if the update constitute a breaking change or not.

Regarding the update of the docs.onyxia.sh website, there's two approach.

image

Thanks again for your contribution.

@johnksv
Copy link
Contributor Author

johnksv commented Oct 13, 2023

OK, I'll bump the chart version again, and write some doc (got your invitation)

@johnksv
Copy link
Contributor Author

johnksv commented Oct 13, 2023

Added a "change request" in gitbook, @garronej . Feel free to edit the docs (and this PR of course) accordingly

@olevitt olevitt requested review from garronej and removed request for olevitt October 17, 2023 13:07
@garronej
Copy link
Contributor

Sorry for the delay, I'll fix the conflict and merge asap

@garronej garronej merged commit 09d72b9 into InseeFrLab:main Oct 23, 2023
5 checks passed
@olevitt olevitt mentioned this pull request Oct 23, 2023
@johnksv johnksv deleted the helm-chart-service-account-refactor branch October 24, 2023 08:31
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.

3 participants