search

berkeley-dsep-infra / hubploy

Toolkit to deploy many z2jh based JupyterHubs
BSD 3-Clause "New" or "Revised" License
17 stars 15 forks source link

Add two additional helm options #35

Closed tjcrone closed 5 years ago

tjcrone commented 5 years ago

I think it might make more sense to have hubploy options parallel helm's. Open to doing this differently. Please let me know!

jhamman commented 5 years ago

Late to the party here but thought I'd say that I think these sorts of configuration options would fit really well in the hubploy.yaml config files. For example:

images:
  ...
cluster:
  ...
deploy:
  extra_helm_args: "--timeout 1200 --force --recreate_pods"

This would allow for per-deployment customization of deploy arguments.

tjcrone commented 5 years ago

Great suggestion @jhamman. This is certainly another way to go with this. However with the current changes, per-deployment customization would go into the CircleCI config. I think your suggestion is an intriguing different pathway. @yuvipanda, what do you think?

tjcrone commented 5 years ago

Original discussion on how to make this happen here: yuvipanda/hubploy/issues/34.

yuvipanda commented 5 years ago

@jhamman that's an interesting idea, but I think @tjcrone has convinced me that we should have individual settings for what we want than something en-masse. I'm also wary of setting --force as something that's done on each deploy automatically - for example, it could theoreticlaly delete & recreate your hub pod's disk - or worse, your user's disk (losing all data). It does make sense for timeout (or even recreate-pods, if there's a need) to be there though.

What do you think, @tjcrone?

tjcrone commented 5 years ago

Well, I think both methods seem to offer similar flexibility in terms of per-deployment settings, with one method putting the directive into the CircleCI config, and the other into hubploy.yaml. I agree with @yuvipanda that defined options are a bit safer. Since this one is done and merged, I think we have our answer! Thanks!

jhamman commented 4 years ago

Another downside to the approach chosen here is that you'll have to add new command line args to hubploy for every helm option that you want to pass through (e.g. --recreate-pods). Since hubploy doesn't do anything with these, I don't see why they need to be explicitly exposed in the hubploy interface.

It may also be worth coming up with a philosophy for what the hubploy.yaml file is for. A personal peeve of mine is when configuration options are split between the command line and various config files. As you can tell, my personal preference would be to lean on the config file as the place where options like this go.

yuvipanda commented 4 years ago

@jhamman I agree re: config file vs commandline. For me, it's 'one time use on my commandline, interactively' vs 'repeatedly, every time, on CI'.

For example, I think something like '--dry-run' should be in hubploy.yaml, but timeout should definitely be. --force should also be in commandline, since I don't think it is safe to do that in CI/CD. I guess it becomes a judgement call at this point, but might be useful to write this down?

Does this make sense? Or does it feel like I've too many specific opinions?