Add --teams flag to assign team name to stack. #11974

RobbieMcKinstry · 2023-01-24T21:54:31Z

Description

This PR adds a --teams flag to pulumi stack init which accepts a string. This flag can be provided multiple times. Each team that is provided is assigned read/write permissions on the stack after it has been initialized.

Fixes #10784

Checklist

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

RobbieMcKinstry · 2023-01-24T21:55:07Z

I think I should test this with a mocked out HTTP client and inspect the request to ensure everything is wired together correctly.

pulumi-bot · 2023-01-24T21:55:09Z

Changelog

[uncommitted] (2023-03-21)

Features

[backend/service] Add "--teams" flag to assign team name to stack during init
#11974

RobbieMcKinstry · 2023-01-24T21:56:18Z

Also, pulumi new should support --teams as well.

pkg/backend/filestate/backend_test.go

pkg/cmd/pulumi/stack_init_test.go

pkg/cmd/pulumi/stack_init.go

pkg/backend/httpstate/backend.go

pkg/cmd/pulumi/stack_init.go

pkg/backend/filestate/backend_test.go

pkg/backend/backend.go

pkg/backend/filestate/backend.go

pkg/backend/filestate/backend_test.go

pkg/cmd/pulumi/stack_init.go

pkg/cmd/pulumi/stack_init_test.go

This PR adds a --teams flag to `pulumi stack init` which accepts a string. This flag can be provided multiple times. Each team that is provided is assigned read/write permissions on the stack after it has been initialized.

RobbieMcKinstry · 2023-03-17T21:02:18Z

I've addressed all feedback on this PR, and I think it's ready to go. Looking for a final review/final feedback comments before we merge.

The Service team has asked that we get this in urgently, as the end of the quarter is fast approaching.

abhinav · 2023-03-17T22:14:26Z

pkg/backend/backend.go

+type CreateStackOptions interface {
+	// Teams returns the list of teams who should have access to
+	// the newly created stack. This method is only approperate
+	// for backends which support teams (i.e. the Service).
+	Teams() []string
+}


Missed this discussion earlier in #11974 (comment) earlier.
Looking at the current version of the code:
I'm still not sure that the interface and the complexity its bringing buys us anything.
It doesn't actually give us make illegal states unrepresentable as suggested because there's only one real implementation that acts the same was as a plain CreateStackOptions struct would.
httpstate and filestate check the result of Teams() the same way they would if it was a struct.
"Accept interfaces, return structs" is good advice for when the object has any behavior. These are just data objects with zero behavior.

I want to repeat my advise here on using a simple *CreateStackOptions struct pointer argument.
That has a couple benefits. First, callers can safely pass in nil when they don't care and the consumer of the options can just do if opts == nil { opts = &CreateStackOptions{}, versus the current situation where callers either have to always pass a non-nil implementation or the consumer has to have a no-op implementation sitting around.
Second, and this is super minor, it avoids need of coming up with a second name for CreateStackOptions. (I find StandardCreateStackOptions to be a bit awkwardly named.)

// pkg/backend.go type CreateStackOptions struct { Teams []string } type Backend interface { // ... CreateStack(ctx context.Context, stackRef StackReference, root string, opts *CreateStackOptions) (Stack, error) // ... }

// pkg/filestate/backend.go func (b *localBackend) CreateStack(ctx context.Context, stackRef backend.StackReference, root string, opts *backend.CreateStackOptions, ) (backend.Stack, error) { // Before locking the stack, validate that the options provided are valid. if opts != nil && len(opts.Teams) > 0 { return nil, &internalIllegalTeamsError{} }

// pkg/httpstate/backend.go func (b *cloudBackend) CreateStack( ctx context.Context, stackRef backend.StackReference, root string, opts *backend.CreateStackOptions, ) ( backend.Stack, error, ) { if opts == nil { opts = &backend.CreateStackOptions{} } // ... apistack, err := b.client.CreateStack(ctx, stackID, tags, opts.Teams)

I definitely disagree here. But instead of relitigating the merits, I'll make the change in a follow-on PR.

Since your objection isn't a matter of correctness and given that @justinvp has asked we get this feature in ASAP,
I'm afraid I have to insist we leave it as-is in this PR, but I'll open an issue and create a follow-on PR today to change the type from an interface into a struct.

@RobbieMcKinstry
The tests are all still panicking with nil pointer dereferences.

panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x284be1e] goroutine 30 [running]: testing.tRunner.func1.2({0x32d8680, 0x6f046a0}) /opt/hostedtoolcache/go/1.20.2/x64/src/testing/testing.go:1526 +0x372 testing.tRunner.func1() /opt/hostedtoolcache/go/1.20.2/x64/src/testing/testing.go:1529 +0x650 panic({0x32d8680, 0x6f046a0}) /opt/hostedtoolcache/go/1.20.2/x64/src/runtime/panic.go:890 +0x263 github.com/pulumi/pulumi/pkg/v3/backend/httpstate.(*cloudBackend).CreateStack(0xc000938a80, {0x5be8a00, 0xc000136008}, {0x5be7d00, 0xc000f16bc0}, {0xc001b28c60, 0x17}, {0x0, 0x0}) /home/runner/work/pulumi/pulumi/pkg/backend/httpstate/backend.go:840 +0x1fe

The change I'm suggesting is trivial to make and will help address all of these panics.

I agree with the feedback to use a simple struct here (*CreateStackOptions). I'd expect it'd be a pretty quick change to make as part of this PR today. No need to do it as a follow-up.

pkg/backend/filestate/backend.go

pkg/backend/filestate/backend_test.go

pkg/cmd/pulumi/stack_init_test.go

Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>

abhinav · 2023-03-20T15:42:28Z

@RobbieMcKinstry Did you mean to push more code? The code for a lot of the comments marked resolved is unchanged.

This commits addresses PR feedback to prefer using a contract assertion instead of returning an internal error. It also adds a test to validate how the --team flag is handled during stack creation when the backend doesn't support teams. Currently, the newly added test is expected to fail.

The previous iteration of the test failed, and I'm not sure why. This commit weakens the assertion, checking only for the presence of an Error, not inspecting the contents.

RobbieMcKinstry · 2023-03-20T16:17:45Z

@RobbieMcKinstry Did you mean to push more code? The code for a lot of the comments marked resolved is unchanged.

@abhinav I took one of your code comments -- a typo in aingle => single and that kicked off another CI job 😆

RobbieMcKinstry · 2023-03-20T16:18:23Z

I worked on addressing the comments locally and pushed a few commits to address. Going to look to see why CI is failing.

RobbieMcKinstry · 2023-03-20T16:19:14Z

Looks like another test elsewhere needs to be updated. On it.

Per discussion, turn CreateStackOptions into a struct with a Teams field.

RobbieMcKinstry · 2023-03-21T17:08:56Z

@abhinav that should be everything.

Frassle

Looks fine to me

Frassle · 2023-03-21T17:29:29Z

pkg/cmd/pulumi/stack_init.go

+
+// TeamsUnsupportedError is the error returned when the --teams
+// flag is provided on a backend that doesn't support teams.
+type teamsUnsupportedError struct {


Not sure we really need an error struct for this? Not sure in general what our leaning is vis-a-vis making new error types or just using fmt.Errorf actually...

Generally, we don't need to bother with new error types for short-enough messages,
where the caller doesn't need to match on that exact object and extract information
(relevant ref).
However, if the error message is complex/has a bunch of conditionals to determine what gets put in it,
an error object is worth.

In this case, we don't need it, but I don't consider it a blocker by any means.

Let's discuss in ideation.

RobbieMcKinstry · 2023-03-21T17:49:52Z

bors r+

RobbieMcKinstry · 2023-03-21T17:51:42Z

Agh! Now the linter failed!

bors · 2023-03-21T18:07:40Z

Canceled.

abhinav

LGTM

abhinav · 2023-03-21T18:33:25Z

pkg/cmd/pulumi/stack_init.go

+
+// TeamsUnsupportedError is the error returned when the --teams
+// flag is provided on a backend that doesn't support teams.
+type teamsUnsupportedError struct {


Generally, we don't need to bother with new error types for short-enough messages,
where the caller doesn't need to match on that exact object and extract information
(relevant ref).
However, if the error message is complex/has a bunch of conditionals to determine what gets put in it,
an error object is worth.

In this case, we don't need it, but I don't consider it a blocker by any means.

RobbieMcKinstry · 2023-03-21T19:36:36Z

bors r+

bors · 2023-03-21T20:08:35Z

Build succeeded:

bors-ok

This deletes the SupportsTeams method added in #11974. It came up during review that we want to avoid too many new "SupportsFoo" methods. Instead, we'll let each backend report whether it supports teams by returning ErrTeamsNotSupported. As a result of this change, validateCreateStackOpts cannot error, so it's been renamed to newCreateStackOptions. Testing: There's already a test (added in the #12499 refactor) that verifies that we report the appropriate error when the backend doesn't support --teams. This updates the mock in that test.

12551: pkg/backend: Delete SupportsTeams methods r=abhinav a=abhinav This deletes the SupportsTeams method added in #11974. It came up during review that we want to avoid too many new "SupportsFoo" methods. Instead, we'll let each backend report whether it supports teams by returning ErrTeamsNotSupported. As a result of this change, validateCreateStackOpts cannot error, so it's been renamed to newCreateStackOptions. Testing: There's already a test (added in the #12499 refactor) that verifies that we report the appropriate error when the backend doesn't support --teams. This updates the mock in that test. Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>

RobbieMcKinstry force-pushed the mckinstry/#10784 branch from c984453 to ddaaba5 Compare January 24, 2023 21:56

Frassle reviewed Jan 24, 2023

View reviewed changes

pkg/backend/filestate/backend_test.go Outdated Show resolved Hide resolved

pkg/cmd/pulumi/stack_init_test.go Outdated Show resolved Hide resolved

RobbieMcKinstry mentioned this pull request Jan 25, 2023

Allow specifying a team when creating a stack #10784

Closed

abhinav reviewed Jan 30, 2023

View reviewed changes

pkg/cmd/pulumi/stack_init.go Outdated Show resolved Hide resolved

pkg/backend/httpstate/backend.go Outdated Show resolved Hide resolved

pkg/cmd/pulumi/stack_init.go Outdated Show resolved Hide resolved

pkg/backend/filestate/backend_test.go Outdated Show resolved Hide resolved

RobbieMcKinstry force-pushed the mckinstry/#10784 branch from ddaaba5 to a146d28 Compare February 6, 2023 19:39

RobbieMcKinstry marked this pull request as ready for review February 6, 2023 19:41

RobbieMcKinstry requested review from abhinav and Frassle February 8, 2023 15:41

Frassle reviewed Feb 8, 2023

View reviewed changes

Add --teams flag to assign team name to stack.

e7fccef

This PR adds a --teams flag to `pulumi stack init` which accepts a string. This flag can be provided multiple times. Each team that is provided is assigned read/write permissions on the stack after it has been initialized.

RobbieMcKinstry force-pushed the mckinstry/#10784 branch from 734a4c8 to e7fccef Compare March 17, 2023 20:59

abhinav reviewed Mar 17, 2023

View reviewed changes

Update pkg/cmd/pulumi/stack_init_test.go

bc802f1

Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>

RobbieMcKinstry added 3 commits March 20, 2023 11:54

This commit updates the new test to pass.

f47acff

The previous iteration of the test failed, and I'm not sure why. This commit weakens the assertion, checking only for the presence of an Error, not inspecting the contents.

Satisfy linter.

da5ae5f

RobbieMcKinstry and others added 3 commits March 20, 2023 12:30

Guard against nil opts

6939229

Remove guard that's covered elsewhere.

8391548

CreateStackOptions: interface => struct

093dab6

Per discussion, turn CreateStackOptions into a struct with a Teams field.

abhinav force-pushed the mckinstry/#10784 branch from 5667d72 to 093dab6 Compare March 20, 2023 21:32

Change error message punctuation.

4f1cd97

RobbieMcKinstry requested a review from abhinav March 21, 2023 17:09

Frassle approved these changes Mar 21, 2023

View reviewed changes

Fix lint: long line.

8943ba5

abhinav approved these changes Mar 21, 2023

View reviewed changes

bors bot merged commit f7c940e into master Mar 21, 2023
40 checks passed

bors bot deleted the mckinstry/#10784 branch March 21, 2023 20:08

abhinav mentioned this pull request Mar 29, 2023

pkg/backend: Delete SupportsTeams methods #12551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --teams flag to assign team name to stack. #11974

Add --teams flag to assign team name to stack. #11974

RobbieMcKinstry commented Jan 24, 2023 •

edited

RobbieMcKinstry commented Jan 24, 2023

pulumi-bot commented Jan 24, 2023 •

edited

RobbieMcKinstry commented Jan 24, 2023

RobbieMcKinstry commented Mar 17, 2023

abhinav Mar 17, 2023

RobbieMcKinstry Mar 20, 2023

abhinav Mar 20, 2023

justinvp Mar 20, 2023

abhinav commented Mar 20, 2023

RobbieMcKinstry commented Mar 20, 2023

RobbieMcKinstry commented Mar 20, 2023

RobbieMcKinstry commented Mar 20, 2023

RobbieMcKinstry commented Mar 21, 2023

Frassle left a comment

Frassle Mar 21, 2023

abhinav Mar 21, 2023

RobbieMcKinstry Mar 21, 2023

RobbieMcKinstry commented Mar 21, 2023

RobbieMcKinstry commented Mar 21, 2023

bors bot commented Mar 21, 2023

abhinav left a comment

abhinav Mar 21, 2023

RobbieMcKinstry commented Mar 21, 2023

bors bot commented Mar 21, 2023

Add --teams flag to assign team name to stack. #11974

Add --teams flag to assign team name to stack. #11974

Conversation

RobbieMcKinstry commented Jan 24, 2023 • edited

Description

Checklist

RobbieMcKinstry commented Jan 24, 2023

pulumi-bot commented Jan 24, 2023 • edited

Changelog

[uncommitted] (2023-03-21)

Features

RobbieMcKinstry commented Jan 24, 2023

RobbieMcKinstry commented Mar 17, 2023

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

abhinav commented Mar 20, 2023

RobbieMcKinstry commented Mar 20, 2023

RobbieMcKinstry commented Mar 20, 2023

RobbieMcKinstry commented Mar 20, 2023

RobbieMcKinstry commented Mar 21, 2023

Frassle 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

RobbieMcKinstry commented Mar 21, 2023

RobbieMcKinstry commented Mar 21, 2023

bors bot commented Mar 21, 2023

abhinav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbieMcKinstry commented Mar 21, 2023

bors bot commented Mar 21, 2023

RobbieMcKinstry commented Jan 24, 2023 •

edited

pulumi-bot commented Jan 24, 2023 •

edited