Add two additional helm options #35
Conversation
hubploy/__main__.py
Outdated
| @@ -52,6 +52,15 @@ def main(): | |||
| deploy_parser.add_argument( | |||
| '--version', | |||
| ) | |||
| deploy_parser.add_argument( | |||
| '--timeout', | |||
| nargs=1 | |||
There was a problem hiding this comment.
nargs=1 produces a list with one item instead of the value itself. Is there a reason to want to do that?
There was a problem hiding this comment.
I did not know this. I will remove this.
hubploy/__main__.py
Outdated
| ) | ||
| deploy_parser.add_argument( | ||
| '--force', | ||
| nargs=0, |
There was a problem hiding this comment.
You don't need this with action='store_true' I think - it sets the default to false & doesn't allow args to be passed to it.
There was a problem hiding this comment.
I want the default to be false, and we don't want any arguments to --force. Isn't this what we want?
There was a problem hiding this comment.
If we don't use store_true, I will need to set a default value in helm.py, which is fine.
There was a problem hiding this comment.
Yeah, as I look more carefully, it appears that if we do not use store_true, then True would need to be passed, which is not parallel to helm. Helm does not have any arguments to --force. So I think we should keep it like this.
There was a problem hiding this comment.
Ah, sorry for the confusion. I meant the nargs=0 is not necessary with store_true, since it is superfluous. Hope this makes more sense.
tjcrone
force-pushed
the
helm-options
branch
from
38c103c
to
36e0665
Compare
|
Late to the party here but thought I'd say that I think these sorts of configuration options would fit really well in the images:
...
cluster:
...
deploy:
extra_helm_args: "--timeout 1200 --force --recreate_pods"This would allow for per-deployment customization of deploy arguments. |
|
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? |
|
Original discussion on how to make this happen here: /issues/34. |
|
@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? |
|
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! |
|
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. It may also be worth coming up with a philosophy for what the |
|
@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? |
I think it might make more sense to have hubploy options parallel helm's. Open to doing this differently. Please let me know!