-
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
Correctly rename stack files during a rename #5812
Conversation
@chrsmith thoughts on testing strategy for this? I thought we had SaaS-backend tests in this repo, but I've been away for awhile, and can't seem to find them? Am I imagining this? |
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.
@chrsmith thoughts on testing strategy for this? I thought we had SaaS-backend tests in this repo, but I've been away for awhile, and can't seem to find them? Am I imagining this?
The tests in https://github.com/pulumi/pulumi/blob/master/tests/stack_test.go should be able to target either backend, and in particular TestStackRenameAfterCreate
.
That, by the way, causes problems if we have multiple stacks in different organizations that share a stack-part.
FWIW - this is tracked in #5511.
I see, so if the test doesn't include an explicit |
80112ad
to
22e7493
Compare
Ok, @chrsmith or @lukehoban , PTAL, should be ready to go. |
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.
The change seems simple and straight forward enough, though either I'm missing something or there are a few ways we can simplify/clean this up a bit.
pkg/backend/httpstate/backend.go
Outdated
url := "." | ||
errMsg := "New stack owner, %s, does not match existing owner, %s.\n\n" | ||
|
||
qn, _ := b.parseStackName(string(newName)) |
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.
It seems like we should delete lines 787-793.
Unless I'm missing something, newName
doesn't get mutated after line 774, so there is no difference between newRef
introduced earlier and qn
defined here. Right?
Similarly, cloudBackend::ParseStackReference
will always return the fully-qualified stack name (owner/project/name), so it should never be the case that qn.Owner == ""
. (cloudBackend::parseStackName
on the other hand will leave the value as "", since it doesn't infer missing values to fully-qualify the name.)
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.
cloudBackend::parseStackName on the other hand will leave the value as "", since it doesn't infer missing values to fully-qualify the name.
Yes, that's exactly why we're using parseStackName
here, so that we can check whether the owner was left off and give a better error message in that case ("did you forget?") -- because ParseStackReference
populates it.
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.
BTW, the reason I made this change is that I found the current error message confusing. It seems more likely that I've simply forgotten to include the fully qualified name than it does I'm trying to transfer the stack between organizations. In the event I am trying to transfer, the current error is great, but wanted to also cater to the likely common case too.
pkg/backend/httpstate/backend.go
Outdated
|
||
return errors.Errorf(errMsg) | ||
return nil, errors.Errorf(errMsg, stackID.Owner, newIdentity.Owner) |
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'd suggest we just use errors.New
here, and use errMsg := fmt.Spritnf(..., stackID.Owner, newIdentity.Owner)
earlier. That way we don't need to "remember" there are 2x values to be filled into the error string later.
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.
Sounds good.
pkg/backend/httpstate/backend.go
Outdated
|
||
url := "." |
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: Rather than url
being either "." or ":\n\n ", why don't we rename this to errMsgSuffix
instead?
That makes it clearer that it isn't actually a URL, but rather some text that gets added to the end of the error message, and so it seems a little less confusing that it gets initialized to ".".
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.
Happy to do that and leave the code better than it was -- this was just how it was written before.
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.
Anyways, these changes look good assuming you take a look and apply whatever cleanups as appropriate.
Just so I understand, the next step towards cleaning up the "stack owner x project name x Pulumi.project-name.yaml" is to update callers of RenameStack(...)
to refer to the newly returned StackReference
, and perhaps be smarter about updating the Pulumi.*.yaml
file and/or persisting more information inside of that?
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.
Thanks for making the changes. All is well, with some tiny nitpicks if you are interested.
Yes, that's exactly why we're using parseStackName here, so that we can check whether the owner was left off...
Ah, I get it now. Thanks. Having a more specific error message like "did you mean..." sounds great.
Two very minor things though:
qn, _ := b.parseStackName(string(newName))
Is qn
short for qualified name? If so, part of what confused me was that we call parseStackName
explicitly because it is not qualified. So maybe leaving a comment to call out this non-obvious expectation?
Also, I thought the Golang linter would complain if you ever swallowed an error
return value from a function. So I'm surprised it isn't complaining about qn, _ :=
. That having been said, I won't harp on you for not adding tedious and likely unnecessarily excessive error handling.
This fixes #4463, by renaming a stack's configuration file based on its stack-part, and ignoring the owner-part. Our workspace system doesn't recognize configuration files with fully qualified names. That, by the way, causes problems if we have multiple stacks in different organizations that share a stack-part. The fix here is simple: propagate the new StackReference from the Rename operation and rely on the backend's normalization to a simple name, and then use that the same way we are using a StackReference to determine the path for the origin stack. An alternative fix is to recognize fully qualified config files, however, there's a fair bit of cleanup we will be doing as part of #2522 and #4605, so figured it is best to make this work the way the system expects first, and revisit it as part of those overall workstreams. I also suspect we may want to consider changing the default behavior here as part of #5731. Tests TBD; need some advice on how best to test this since it only happens with our HTTP state backend -- all integration tests appear to use the local filestate backend at the moment.
98f0b8b
to
0acf8cd
Compare
Use "parsedName" instead of "qn", add a comment explaining why we're doing this, and also explicitly ignore the error rather than implicitly doing so with _.
b570b2e
to
f9ba25f
Compare
Fair feedback! This is actually a qualified name (it includes the |
This fixes #4463, by renaming a stack's configuration file based on its stack-part, and ignoring the owner-part. Our workspace system doesn't recognize configuration files with fully qualified names. That, by the way, causes problems if we have multiple stacks in different organizations that share a stack-part.
The fix here is simple: propagate the new StackReference from the Rename operation and rely on the backend's normalization to a simple name, and then use that the same way we are using a StackReference to determine the path for the origin stack.
An alternative fix is to recognize fully qualified config files, however, there's a fair bit of cleanup we will be doing as part of #2522 and #4605, so figured it is best to make this work the way the system expects first, and revisit it as part of those overall workstreams. I also suspect we may want to consider changing the default behavior here as part of #5731.
Tests TBD; need some advice on how best to test this since it only happens with our HTTP state backend -- all integration tests appear to use the local filestate backend at the moment.