Revise the way previews are controlled #1290
Conversation
|
@lukehoban @CyrusNajmabadi I am very open to feedback on this change. I originally just renamed the flags, but then realized the need for option (2) -- this is what LM, and I suspect most CI folks, will do -- and the illegal combinations of flags felt really kludgy. At the same time, |
FWIW - In our own CI (integration test framework) we accomplish this via Is there a simpler change where we just change the name of |
|
That's where I started, but it just felt kludgy. All the places where we check for bad combinations of flags, etc. Plus, a benefit I had hoped this would lead to is not having to run the command twice (this was at least 50% of why I was excited about the change in the first place!) |
|
Just to summarize for myself - here's the four common cases I can imagine:
FWIW - It's partly the fact that I believe (3) will be the proper way to do CI/CD longterm (fundamentally two different steps), that makes me less worried about (2) being two steps as well - and in fact lightly prefer that these two feel more similar (in fact, identical in our existing model) across the two CI/CD workflows. Net, while I understand the motivation for the change, I don't feel like the "afters" above are clearly better than the "befores". Thoughts? (I would still look for another word instead of Would also love @CyrusNajmabadi input, as he's thought a fair bit about this as well. |
|
I suspect things will change even more once we get to sophisticated approval workflows. The thing I hate about all of this is that we're using flags to perform logically distinct actions. That feels awkward to me, and screams out for commands. What if we
Going back to your scenarios, we then end up with
|
|
By the way, when I said
I say this because I expect us to want something entirely different. For example: where |
|
I'm not sure how i feel aobut this. It only seems to affect hte following case: "i want to auto approve if there was no issue". Which only saves slightly over "just run normal update, and then approve manually" or just running "--preview" then "--force". Noodling on this a bit to see if anything comes to mind as strictly superior. |
|
I think part of my issue is this:
The difference between these is just too subtle for me to think is valuable to give to a user. auto-accept essentially means: "go do it" and "skip" just means "go do it". So really, all that happens here is if the preview is printed out or not... which seems pretty minor and unessential a thing to provide control over. It would likely also have a difference if the preview caused an error. however, that's also extremely subtle. |
|
After using the new system for a few days, here's some feedback:
What Joe outlines in his last message feels right to me. Instead of |
I like this. |
|
All that sounds great. Thanks, I'll proceed with this set of updates. |
|
A quick question for folks. It's annoying that commands like |
joeduffy
force-pushed
the
preview_flag
branch
8 times, most recently
from
6268214
to
cdcb987
Compare
I found the flag --force to be a strange name for skipping a preview, since that name is usually reserved for operations that might be harmful and yet you're coercing a tool to do it anyway, knowing there's a chance you're going to shoot yourself in the foot. I also found that what I almost always want in the situation where --force was being used is to actually just run a preview and have the confirmation auto-accepted. Going straight to --force isn't the right thing in a CI scenario, where you actually want to run a preview first, just to ensure there aren't any issues, before doing the update. In a sense, there are four options here: 1. Run a preview, ask for confirmation, then do an update (the default). 2. Run a preview, auto-accept, and then do an update (the CI scenario). 3. Just run a preview with neither a confirmation nor an update (dry run). 4. Just do an update, without performing a preview beforehand (rare). This change enables all four workflows in our CLI. Rather than have an explosion of flags, we have a single flag, --preview, which can specify the mode that we're operating in. The following are the values which correlate to the above four modes: 1. "": default (no --preview specified) 2. "auto": auto-accept preview confirmation 3. "only": only run a preview, don't confirm or update 4. "skip": skip the preview altogether As part of this change, I redid a bit of how the preview modes were specified. Rather than booleans, which had some illegal combinations, this change introduces a new enum type. Furthermore, because the engine is wholly ignorant of these flags -- and only the backend understands them -- it was confusing to me that engine.UpdateOptions stored this flag, especially given that all interesting engine options _also_ accepted a dryRun boolean. As of this change, the backend.PreviewBehavior controls the preview options.
This changes the CLI interface in a few ways:
* `pulumi preview` is back! The alternative of saying
`pulumi update --preview` just felt awkward, and it's a common
operation to want to perform. Let's just make it work.
* There are two flags consistent across all update commands,
`update`, `refresh`, and `destroy`:
- `--skip-preview` will skip the preview step. Note that this
does *not* skip the prompt to confirm that you'd like to proceed.
Indeed, it will still prompt, with a little warning text about
the fact that the preview has been skipped.
* `--yes` will auto-approve the updates.
This lands us in a simpler and more intuitive spot for common scenarios.
|
I've gone with this latest idea, of not requiring |
lindydonna
added
the
impact/breaking
|
I'll def have to try this out. However, a lot of the tweaks seem sensible to me. Especially about having defualt behavior in non-tty sessions. The tty seems like the place where we can say "we're interactive with teh user, not a script, so we can def involve them more." If it's a script, we can just presume the user knows what they're doing and proceed. |
I found the flag
--forceto be a strange name for skipping a preview,since that name is usually reserved for operations that might be harmful
and yet you're coercing a tool to do it anyway, knowing there's a chance
you're going to shoot yourself in the foot.
I also found that what I almost always want in the situation where
--forcewas being used is to actually just run a preview and have theconfirmation auto-accepted. Going straight to
--forceisn't the rightthing in a CI scenario, where you actually want to run a preview first,
just to ensure there aren't any issues, before doing the update.
In a sense, there are four options here:
This change enables all four workflows in our CLI.
Rather than have an explosion of flags, we have a single flag,
--preview, which can specify the mode that we're operating in. The
following are the values which correlate to the above four modes:
"": default (no --preview specified)"auto": auto-accept preview confirmation"only": only run a preview, don't confirm or update"skip": skip the preview altogetherAs part of this change, I redid a bit of how the preview modes
were specified. Rather than booleans, which had some illegal
combinations, this change introduces a new enum type. Furthermore,
because the engine is wholly ignorant of these flags -- and only the
backend understands them -- it was confusing to me that
engine.UpdateOptionsstored this flag, especially given that allinteresting engine options also accepted a
dryRunboolean. As ofthis change, the
backend.PreviewBehaviorcontrols the preview options.