-
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
Switch to a resource-progress oriented view for pulumi preview/update/destroy #1116
Conversation
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.
I think at a minimum this is going to give the old behavior for stacks managed by the service and we'll want to address that before we land. I left a lot of other feedback, but it was mainly style things.
pkg/backend/local/stream.go
Outdated
) | ||
|
||
// copied from: https://github.com/docker/cli/blob/master/cli/command/out.go | ||
// replace with usage of that library when we can figure out hte right version story |
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.
nit: typo, "teh" should be "the"
@@ -206,7 +206,7 @@ func newConfigSetCmd(stack *string) *cobra.Command { | |||
// If we saved a plaintext configuration value, and --plaintext was not passed, warn the user. | |||
if !secret && !plaintext { | |||
cmdutil.Diag().Warningf( | |||
diag.Message( | |||
diag.Message("", /*urn*/ |
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.
Do you think it is worthwhile to have diag.Message
and diag.URNMessage
, so we can centralize the empty URN dance to a single location instead of throwing it in every call site?
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.
@ellismg I'm on the fence. One of the nice things about this approach was that it forced me to go look at every usage of Message to ensure that we at least think about what we're trying to pass along. However, now that we have examined all usages, we can now try to make this easier to use in teh future. However, part of me still likes making the caller have to think about this sort of thing.
pkg/backend/cloud/state.go
Outdated
@@ -184,10 +186,9 @@ func (u *cloudUpdate) RecordAndDisplayEvents(action string, | |||
|
|||
// Start the local display processor. | |||
displayEvents := make(chan engine.Event) | |||
go local.DisplayEvents(action, displayEvents, done, debug, opts) | |||
go local.DisplayDiffEvents(action, displayEvents, done, debug, opts) |
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.
I think you want to change this line so that it defaults to DisplayProgressEvents
or DisplayDiffEvents
depending on the command line. This call here is the thing that is showing messages locally, so as written I think we'll continue to use the old style for hosted stacks, which is not what we want.
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.
Gotcha. Will fix.
var ( | ||
// simple regex to take our names like "aws:function:Function" and convert to | ||
// "aws:Function" | ||
typeNameRegex = regexp.MustCompile("^(.*):(.*):(.*)$") |
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.
I think because of greedy matching, if presented with something like:
one:two:three:four
We'll end up giving back one:two:three:four
, we'll end up giving back one:two:four
which is unlikely what we want.
We can either use non-greedy matching (e.g: regexp.MustCompile("^(.*?):(.*?):(.*?)$")
) or perhaps use the methods on the tokens.Type
object?
If it's not possible to have a tokens.Type that has more than two colons, I guess we are safe. I'm not sure if such a thing is possible today.
Using the methods may still be nicer anyway, since we don't have to convert to a string and then regexp.
return payload.URN | ||
} | ||
|
||
return "" |
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.
Given the above comment, it sounds like we should never reach this case? If so, can we add an assert?
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.
fixed comment. it was wrong.
pkg/backend/local/progress.go
Outdated
|
||
// Once we've written everything to the pipe, we're done with it can let it go. | ||
err := pipeWriter.Close() | ||
if err != nil { |
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.
NIT: if we are ignoring the error anyway, don't have this if condition. contract.IgnoreError does the right thing when the error is nil.
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.
done. thanks!
pkg/backend/local/progress.go
Outdated
} | ||
|
||
printStatusForTopLevelResource := func(status Status) { | ||
writeAction := func(msg string) { |
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.
Can we restructure this such that we don't have a local method that itself has a local function? e.g. could we move the code that follows this up to before it and have it just set a msg
local that the rest of the function uses?
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.
sure!
pkg/backend/local/progress.go
Outdated
|
||
func renderProgressEventWorker( | ||
event engine.Event, seen map[resource.URN]engine.StepEventMetadata, | ||
debug bool, opts backend.DisplayOptions, isPreview bool) string { |
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.
Should we try to push the debug
flag into the backend.DisplayOptions
bag?
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. i would like that. i'm not a fan that in most of the engine it seems like we have 2-3 places to get options (often several parameters, and sometimes off of the 'this' object).
pkg/backend/local/progress.go
Outdated
isPreview bool, isComplete bool) string { | ||
|
||
out := &bytes.Buffer{} | ||
summary := getMetadataSummaryWorker(metadata, isPreview, isComplete) |
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.
FWIW, I think the Worker
suffix here is confusing (I would expect the worker to actually do some long running operation). Can we just inline the worker into getMetadataSummary? Similar feedback for the other Worker methods you've introduced. They only ever look like they are called from one place and I'd personally find it clearer if the code was inline.
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.
sure.
pkg/backend/local/progress.go
Outdated
return op.Prefix() + getStepCompleteDescriptionNoColor(op) + colors.Reset | ||
} | ||
|
||
func getStepDescription(op deploy.StepOp, isPreview bool) string { |
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.
Again, my preference would be to inline this into the above (assuming it's the only place we call this, which it looks like is the case)
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.
sure!
Merging in. Let me know if there are any changes you want and let me know how things feel. Note: if you want the old experience, just pass --diff to "pulumi update/preview/destroy", othewise you'll get the new expeirence. |
11122: [SDK/Python] Respect --parallel flag r=RobbieMcKinstry a=RobbieMcKinstry <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description This PR sets the default futures executor to use the `--parallel` flag to determine thread count. <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #1116 ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works. **(Not unit tests, but I ran a thorough experiment.)** <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Robbie McKinstry <robbie@pulumi.com>
No description provided.