-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Bring pulumi preview
in-line with pulumi up
#3675
Conversation
Adds support to `pulumi preview` to match `pulumi up` for: * `refresh` * `target` * `replace` * `target-replace` * `target-dependents` Fixes #3674.
@@ -105,7 +103,7 @@ func newPreviewCmd() *cobra.Command { | |||
return result.FromError(err) | |||
} | |||
|
|||
m, err := getUpdateMetadata("", root) | |||
m, err := getUpdateMetadata(message, root) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change to align with up
that's a bit unrelated, but I made as part of doing a pass to align these. @chrsmith Is this change correct, or is it intentional that we pass an empty message on standalone pulumi preview
(vs. on the preview associated with a pulumi up
)?
Notably - we support a --message
parameter to pulumi preview
but were previously ignoring it - so I'm nearly certain this was just wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, passing the empty string/ignoring it is just plain wrong.
This is the seldom used, but super-useful feature where you can provide a custom message to an update operation. (Since we default to the git commit message, it isn't going to be useful for the local development story if you run pulumi up
several times from the same commit.) So pulumi up -m "Add IAM role permissions"
will make matching the actual update correct in the console.
If we can hook that up for both previews and updates, that would be great. (Though in theory, we'd probably want to do it for refreshes and destroys too... Feel free to file an issue and I can make the cleanup.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though in theory, we'd probably want to do it for refreshes and destroys too...
It appears we already do this correctly for refresh and destroy - it looks like pulumi preview
was the only one that had this wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If you want to go for the gold (@chrsmith™), it would be nice to display the "details" prompt at the end of the preview when run interactively (ala update).
Adds support to
pulumi preview
to matchpulumi up
for:refresh
target
replace
target-replace
target-dependents
Fixes #3674.