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

Chart : resources by default #644

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Chart : resources by default #644

merged 4 commits into from
Nov 16, 2023

Conversation

olevitt
Copy link
Contributor

@olevitt olevitt commented Oct 27, 2023

Exact values are up for discussion

@garronej
Copy link
Contributor

Hey @olevitt,
It seems both a little high for the API and low for the WEB but really I have no idea.
Onyxia web, despide being an SPA serves a lot of little files beacause the APP is split into multiple little files that are downloaded lazily. I'm surprised that Ngnix can conforably serves all that with so few resources but really I have no real idea, you are much more experienced with production that I am so I trust your jugement on this (and I'm too lazy to mesure).

If @fcomte is ok let's merge

@olevitt olevitt marked this pull request as ready for review November 14, 2023 07:38
@olevitt
Copy link
Contributor Author

olevitt commented Nov 14, 2023

@garronej this PR is now ok to merge.
We should add a note to the changelog indicating that the Chart now has default resources requests that may or may not suit users needs. How to do that ?

@odysseu
Copy link
Contributor

odysseu commented Nov 16, 2023

@garronej this PR is now ok to merge. We should add a note to the changelog indicating that the Chart now has default resources requests that may or may not suit users needs. How to do that ?

@garronej
Copy link
Contributor

We should add a note to the changelog indicating that the Chart now has default resources requests that may or may not suit users needs. How to do that ?

I take care of it!

@garronej
Copy link
Contributor

For end-users it could be to add a banner : https://github.com/InseeFrLab/onyxia/blob/main/web/.env#L583-L615
Even though any end-user will see the resources's sliders' values. 👀

I think it's more for the instance administrator that for the end user. I guess a notice in the migration guide to v7 should be enough.

helm-chart/Chart.yaml Outdated Show resolved Hide resolved
@garronej garronej merged commit 372b805 into main Nov 16, 2023
2 checks passed
@garronej garronej deleted the chart-resources branch February 7, 2024 14:02
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