pangeo-data / helm-chart

Pangeo helm charts
https://pangeo-data.github.io/helm-chart/
21 stars 26 forks source link

remove resource specs #33

Closed rabernat closed 5 years ago

rabernat commented 6 years ago

After some discussion with @tjcrone, I am starting to think that the resource specs should be specified in the deployment-specific config (e.g. jupyter-config.yaml for pangeo.pydata.org), rather than here in the generic helm chart. We override them there already, so their presence here is confusing.

jacobtomlinson commented 6 years ago

I disagree that we should remove these as failing to set these specs can cause knock on issues such as autoscaling not working properly.

I agree that it is unclear and that we override regularly but perhaps the values should be updated to be suit more use cases and the docs should be updated.

mrocklin commented 6 years ago

My understanding was that in order to serve minikube users the standard was to not include resources in helm charts

On Wed, May 23, 2018 at 4:56 AM, Jacob Tomlinson notifications@github.com wrote:

I disagree that we should remove these as failing to set these specs can cause knock on issues such as autoscaling not working properly.

I agree that it is unclear and that we override regularly but perhaps the values should be updated to be suit more use cases and the docs should be updated.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pangeo-data/helm-chart/pull/33#issuecomment-391272833, or mute the thread https://github.com/notifications/unsubscribe-auth/AASszMG9hAtzYYH55m0NUjHR0G8BgbMUks5t1SQhgaJpZM4UJnAQ .

rabernat commented 6 years ago

I disagree that we should remove these as failing to set these specs can cause knock on issues such as autoscaling not working properly.

But is anyone deploying this chart directly? All the pangeo cloud deployments I know of overwrite these values. We could simply state in the docs that they need to be specified in the deployment-specific config.

But unfortunately, there are no docs.

jacobtomlinson commented 6 years ago

There is nothing stopping people deploying this directly, I would expect people to override these values but values should exist to override. I have seen other examples where this section is there but commented out which helps provide an example usage.

By docs in this instance I meant README.

mrocklin commented 6 years ago

I would expect people to override these values but values should exist to override

I think that when I was working on the stable/dask chart maintainers recommended that I comment out resources so that things could be easily tested on minikube, the idea being that any serious use was likely to override them anyway. Thoughts on this? I would find this useful personally. I would probably test things more often locally before deploying to the live cluster. This slightly puts off the need of a staging environment for a bit.

mrocklin commented 6 years ago

I have seen other examples where this section is there but commented out which helps provide an example usage.

I'm in favor of this

jacobtomlinson commented 6 years ago

Great!

rabernat commented 6 years ago

Let's wait until #29 is merged before messing with the charts any more.

mrocklin commented 6 years ago

Waiting on #29 seems to have been less-than-wise. I'm in favor of merging this.

jhamman commented 6 years ago

29 has been merged. Do we want to resolve these conflicts and merge or has this fallen out of favor?

jacobtomlinson commented 6 years ago

After researching this more I retract my previous concerns. I would be happy for this to be resolved and merged.

jhamman commented 5 years ago

I'm clearing out some old issues. I plan to merge this if the CI tests pass.