Check the uniqueness of the project name during `pulumi new` #3065

mikhailshilkov · 2019-08-12T09:17:30Z

During pulumi new, check if there is an existing project with the name equal to the entered value, and prevent going forward if so.

@justinvp I wasn't able to find any existing tests for the parts that I touched. Let me know if you think we should have some tests here.

justinvp

Some comments, but otherwise LGTM. Thanks!

I wasn't able to find any existing tests for the parts that I touched. Let me know if you think we should have some tests here.

Test coverage has been lacking in this area and is something we want to improve. Tests are always welcome, but I understand it may be a larger effort to get some initial test helpers/infrastructure in place to more easily test this command incrementally going forward. So unless you see a way to add some simple tests now, we can just track getting that in place as a separate effort.

cmd/new.go

pkg/backend/filestate/backend.go

justinvp · 2019-08-13T14:54:02Z

pkg/backend/httpstate/backend.go

@@ -473,6 +473,16 @@ func (b *cloudBackend) Logout() error {
 	return workspace.DeleteAccessToken(b.CloudURL())
 }

+// DoesProjectExist returns true if a project with the given name exists in this backend, or false otherwise.
+func (b *cloudBackend) DoesProjectExist(ctx context.Context, projectName string) (bool, error) {
+	owner, err := b.client.GetPulumiAccountName(ctx)


When we get to adding the ideal experience for #2421 (prompting for the owner), I think we'll need to modify this to allow passing the owner to DoesProjectExist since this is always only checking if the project exists in the user's account.

Good point. Moreover, this is already a problem now:

We ask for a project name, we get proj1 and it's unique in the user's account.

We ask for a stack name, we get org1/stack1 back.

Project proj1 already exists in org1. Boom.

It seems that we are not able to validate the project name before we get the org name, or am I wrong?

It seems that we are not able to validate the project name before we get the org name, or am I wrong?

Right. I think we just live with this gotcha until #3076 is addressed.

justinvp · 2019-08-13T15:24:04Z

cmd/new.go

@@ -367,6 +371,29 @@ func errorIfNotEmptyDirectory(path string) error {
 	return nil
 }

+func validateProjectName(projectName string, opts display.Options) error {


Note that the pulumi new command does accept a --name (or -n) flag and we validate this early on and exit early if the specified project name isn't valid. But that validation isn't calling the new DoesProjectExist.

So even with the changes in this PR I could do:

pulumi new -n exists

And pulumi will happily continue and prompt for the project description and stack name, and will create the stack in the existing project assuming I choose a stack name that doesn't conflict with an existing stack name in the existing project.

I wonder if we should add another check that calls this function to ensure the project doesn't exist earlier on. Perhaps after this block:

pulumi/cmd/new.go

Lines 121 to 127 in 1a698cb

// If we're going to be creating a stack, get the current backend, which

// will kick off the login flow (if not already logged-in).

if !generateOnly {

if _, err = currentBackend(opts); err != nil {

return err

}

}

Something like:

// Ensure the project doesn't already exist. if !generateOnly && name != "" { if err := validateProjectName(name, opts); err != nil { return err } }

However, when we get to the ideal fix for #2421 (prompting for the owner), we'll probably need to make some modifications to such a check as it'll need an owner to perform the "does exist" check. Perhaps it'd only do the check if it has an owner at this point (e.g. the user is logged in to the cloud backend and isn't a member of any orgs, and hence the user account would be the owner). Or if we decided to add an --owner flag and the flag was specified.

justinvp · 2019-08-13T15:25:41Z

cmd/new.go

+		return err
+	}
+
+	exists, err := b.DoesProjectExist(commandContext(), projectName)


If the --generate-only (or -g) flag is specified, we won't be creating a stack. I wonder if in that case we should just skip getting the backend and calling b.DoesProjectExist?

But the project name still needs to be unique in this case, doesn't it?

But the project name still needs to be unique in this case, doesn't it?

It does, but when -g is specified, no actual stack is going to be created as part of the pulumi new command invocation. I suppose it is likely they'll run pulumi stack init as a next step at some point (or indirectly via running pulumi up and it prompting to create a stack). And having new check to ensure the project is unique before any of these subsequent commands could be helpful. However, by the time they run the command that creates the stack, the projects on the server could have changed (although unlikely), or they'll want to create a stack in a different org, in which case the check in the user's org was pointless and would artificially block them when they just want to emit the project on disk.

So I guess I lean slightly towards not doing the check when -g is specified. It's not creating any stacks, so don't bother with with the uniqueness check. But I could be convinced otherwise.

mikhailshilkov · 2019-08-14T12:34:06Z

@justinvp I feel that there are enough scenario permutations here to warrant spending time on building a test suite. I created two basic tests - could you have a look and check whether they are on the proper level?

I had to mock the console input to be able to provide user's input: otherwise, we'll lose many scenarios.

The next step would be to mock the backend to test the name validation. What would be the optimal way to do so? Mock the whole backend like backend_test does? What's the best way to inject the mock backend? Adding it as an extra parameter of newNewCmdEx will impact lots of code in new and up command, as they all rely on currentBackend now.

justinvp · 2019-08-14T19:11:37Z

I created two basic tests - could you have a look and check whether they are on the proper level?

These look fine to me, but would love @ellismg to take a look.

The next step would be to mock the backend to test the name validation. What would be the optimal way to do so? Mock the whole backend like backend_test does? What's the best way to inject the mock backend? Adding it as an extra parameter of newNewCmdEx will impact lots of code in new and up command, as they all rely on currentBackend now.

@ellismg, what would you recommend?

ellismg · 2019-08-19T18:57:18Z

@ellismg, what would you recommend?

I don't think I have any silver bullets here. I think that mocking the backend is correct (we could probably just return not implemented errors for codepaths we did not think would be hit). As far as injecting it goes, I think that passing it to newNewCmdEx somehow is the best way, but I agree it will be slightly disruptive. I'm not really sure of a better way.

Long term, we probably want the backend injected into any command that needs it, so we could mock more of the interactive portions of the CLI.

mikhailshilkov · 2019-08-19T19:58:18Z

@justinvp @ellismg Thanks a ton for your input!

I've restructured the code a bit further to split the actual function from the command which makes testing and injection a bit easier. I've also injected a mock backend with a naughty assignment - I might refactor it to an argument later on. I added a new test which depends on the mocked backend.

This looks promising, so I'll go ahead and try to create enough test cases to cover the combinations for the issue in question.

mikhailshilkov · 2019-08-20T14:06:09Z

I added several more tests, I believe this is ready for another review

justinvp

LGTM otherwise. Thanks for the tests and getting this infrastructure in place, Mikhail!

cmd/util.go

justinvp · 2019-08-20T15:10:27Z

cmd/new_test.go

+	assert.Equal(t, uniqueProjectName, proj.Name.String())
+}
+
+func TestCreatingProjectWithEnteredName(t *testing.T) {


What is the difference between SpecifiedName and EnteredName? In both of these tests uniqueProjectName is being specified as a the name arg.

Great catch! I wasn't careful enough and let it slip in. The Entered ones shouldn't have name specified.

Just a nit, but can we rename the functions to make the distinction more clear? Maybe something like:

TestCreatingProjectWithArgsSpecifiedName

TestCreatingProjectWithPromptedName

cmd/new_test.go

justinvp

A couple nits on the test names, but LGTM otherwise.

justinvp · 2019-08-21T17:41:01Z

cmd/new_test.go

+	assert.Equal(t, uniqueProjectName, proj.Name.String())
+}
+
+func TestCreatingProjectWithEnteredName(t *testing.T) {


Just a nit, but can we rename the functions to make the distinction more clear? Maybe something like:

TestCreatingProjectWithArgsSpecifiedName

TestCreatingProjectWithPromptedName

mikhailshilkov changed the title ~~Check project existence during pulumi new~~ Check the uniqueness of the project name during pulumi new Aug 12, 2019

mikhailshilkov requested a review from justinvp August 12, 2019 10:11

justinvp approved these changes Aug 13, 2019

View reviewed changes

mikhailshilkov force-pushed the mikhailshilkov/gh-2476 branch from e585529 to ca4e16b Compare August 14, 2019 12:23

mikhailshilkov force-pushed the mikhailshilkov/gh-2476 branch 2 times, most recently from 42e5e55 to 5a8e037 Compare August 19, 2019 15:27

mikhailshilkov requested review from justinvp and ellismg August 20, 2019 14:06

justinvp approved these changes Aug 20, 2019

View reviewed changes

mikhailshilkov mentioned this pull request Aug 21, 2019

--stack option broken in pulumi CLI #2880

Closed

justinvp approved these changes Aug 21, 2019

View reviewed changes

mikhailshilkov added 2 commits August 23, 2019 08:35

Check project existence during pulumi new

c1a5153

Extend the test suite

580819f

mikhailshilkov force-pushed the mikhailshilkov/gh-2476 branch from 270024a to 580819f Compare August 23, 2019 06:39

mikhailshilkov merged commit ccc7ec1 into master Aug 23, 2019

pulumi-bot deleted the mikhailshilkov/gh-2476 branch August 23, 2019 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check the uniqueness of the project name during `pulumi new` #3065

Check the uniqueness of the project name during `pulumi new` #3065

mikhailshilkov commented Aug 12, 2019

justinvp left a comment

justinvp Aug 13, 2019

mikhailshilkov Aug 14, 2019

justinvp Aug 14, 2019

justinvp Aug 13, 2019

justinvp Aug 13, 2019

justinvp Aug 13, 2019

mikhailshilkov Aug 14, 2019

justinvp Aug 14, 2019

mikhailshilkov commented Aug 14, 2019

justinvp commented Aug 14, 2019

ellismg commented Aug 19, 2019

mikhailshilkov commented Aug 19, 2019

mikhailshilkov commented Aug 20, 2019

justinvp left a comment

justinvp Aug 20, 2019

mikhailshilkov Aug 20, 2019

justinvp Aug 21, 2019

mikhailshilkov Aug 21, 2019

justinvp left a comment

justinvp Aug 21, 2019

	// If we're going to be creating a stack, get the current backend, which
	// will kick off the login flow (if not already logged-in).
	if !generateOnly {
	if _, err = currentBackend(opts); err != nil {
	return err
	}
	}

Check the uniqueness of the project name during pulumi new #3065

Check the uniqueness of the project name during pulumi new #3065

Conversation

mikhailshilkov commented Aug 12, 2019

justinvp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailshilkov commented Aug 14, 2019

justinvp commented Aug 14, 2019

ellismg commented Aug 19, 2019

mikhailshilkov commented Aug 19, 2019

mikhailshilkov commented Aug 20, 2019

justinvp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinvp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Check the uniqueness of the project name during `pulumi new` #3065

Check the uniqueness of the project name during `pulumi new` #3065