filestate/internal: Use stack reference, not name #12426

abhinav · 2023-03-13T21:26:29Z

filestate backend currently operates exclusively with stack names.
All its internal pass around just the stack name, and nothing else.
This makes it a bit difficult to add project support to the backend.

This is a refactor in advance of adding project support,
changing the internals of filestate to pass a stack reference around.
It inspects the reference directly for all its operations.

Note: This contains no behavioral changes.
Name and FullyQualifiedName currently both return just the stack name.
In a future change, once project name is incorporated into the object,
FullyQualifiedName will be able to return organization/$project/$name.

Change extracted from #12134

abhinav · 2023-03-13T21:26:45Z

Current dependencies on/for this PR:

master
- PR sdk/common: Add testing/diagtest package #12424
  - PR filestate: Plumb context through constructor #12425
    - PR filestate/internal: Use stack reference, not name #12426 👈
      - PR filestate: Introduce referenceStore to control layout #12436
        
        PR backend/filestate: Re-add project support #12437
        
        PR {cli, filestate}: Add 'state upgrade' command #12438
      - PR WIP: Readd project support to filestate #12134

This comment was auto-generated by Graphite.

pulumi-bot · 2023-03-13T21:27:31Z

Changelog

[uncommitted] (2023-03-16)

Frassle

Looks as I'd expect but it's late and I don't trust my eyes to have not missed anything!
Will pick this up in the morning if no one beats me to approving it.

Frassle · 2023-03-13T22:08:26Z

pkg/backend/filestate/backend.go

+	if !is {
+		return nil, fmt.Errorf("bad stack reference type")
+	}
+	return localStackRef, nil


I wonder if this should actually be more strict. In the project changes I added a *localBackend field to localBackendReference and maybe this should check that the references isn't just the right type, but also comes from the backend object that is now trying to use it.

I may have removed the localBackend field. 😆

I suspect the type check here is enough: outside callers cannot construct their own localBackendReference.
I can't imagine many likely scenarios where we'll need to guard against two filestate backends passing bad references to each other. However, if that changes, we can guard against it either in the way you suggest or with a cheap numeric identifier assigned to the localBackend and passed to the localBackendReference.
(I think keeping localBackendReference as a data-only object is desirable until we add meaningfully complex behavior to it.)

Yeh was just a thought, but given we'll need to re-add *localBackend to do the project name lookup in ParseStackReference it would be very doable.

Sorry, I don't understand. Why do we need to add localBackend to localBackendReference?
ParseStackReference is a method on localBackend, so it can do the lookup as needed.

For context, this is the change I made to drop the dependency:
980b3af
localBackendReference held onto localBackend only to look up the current project, so it could shorten the String() representation. I figured it can do that with the name of the current project at creation time instead.
(Accessing localBackend.currentProject on String has implications to thread-safety: there's no guarantee that String is not called concurrently; so if we end up putting the reference back. localBackend.currentProject will probably become an atomic.Pointer.)

Why do we need to add localBackend to localBackendReference?

To match the behavior of httpBackend. You'll change the output of pulumi new if you don't use the current project because we parse the stack reference first then make the project and set it with "SetCurrentProject".

It might be ok to change this, but filestate and httpstate should be the same in regards to this.

Ack. I'll add the behavior back in the filestate PR.
(This PR will be unaffected by that.)

Added back in e3e40b1 and fixed race in c91e493.
(This is just FYI for changes to the draft PR. Real PRs incoming.)

pkg/backend/filestate/backend_test.go

abhinav · 2023-03-15T01:02:15Z

bors merge

12426: filestate/internal: Use stack reference, not name r=abhinav a=abhinav filestate backend currently operates exclusively with stack names. All its internal pass around just the stack name, and nothing else. This makes it a bit difficult to add project support to the backend. This is a refactor in advance of adding project support, changing the internals of filestate to pass a stack reference around. It inspects the reference directly for all its operations. Note: This contains no behavioral changes. Name and FullyQualifiedName currently both return just the stack name. In a future change, once project name is incorporated into the object, FullyQualifiedName will be able to return `organization/$project/$name`. Change extracted from #12134 12433: sdk/go: Don't use Provider for type if package doesn't match r=abhinav a=abhinav In #12296, we started correctly interpreting the Provider argument per other SDKs. but this introduced a regression: the Provider field is now set for resources with mismatched types. This results in a scenario where a provider foo for package X is passed to a resource bar with package Y, with the intent of plumbing it to bar's descendants, but bar attempts to incorrectly use the provider directly. foo := NewXProvider() bar := NewYThing("bar", Provider(foo)) // ... baz := NewXThing("baz", Parent(bar)) // should use foo This worked previously, but with #12296, this fails because NewYThing attempts to use Provider foo directly. To fix this, we need to prevent NewYThing from using a provider that does not match the package that it belongs to. We had to make a similar change to the Python SDK in #12292. https://github.com/pulumi/pulumi/blob/477e0c97660bdbeb28a142057899792d52e63a0b/sdk/python/lib/pulumi/resource.py#L925-L927 The regression test is specific to remote component resources per #12430, but the issue would be caused even for normal component resources before this. Resolves #12430 12435: Pin CI to Node 19.7.x to avoid hitting assert in 19.8.0 r=abhinav a=justinvp Node 19.8.0 was released ~5 hours ago and we're hitting an internal assert. Pin to 19.7.x to unblock CI until we understand what's going on with 19.8.0. Part of #12434 Co-authored-by: Fraser Waters <fraser@pulumi.com> Co-authored-by: Abhinav Gupta <abhinav@pulumi.com> Co-authored-by: Justin Van Patten <jvp@justinvp.com>

bors · 2023-03-15T03:10:50Z

This PR was included in a batch that timed out, it will be automatically retried

12426: filestate/internal: Use stack reference, not name r=abhinav a=abhinav filestate backend currently operates exclusively with stack names. All its internal pass around just the stack name, and nothing else. This makes it a bit difficult to add project support to the backend. This is a refactor in advance of adding project support, changing the internals of filestate to pass a stack reference around. It inspects the reference directly for all its operations. Note: This contains no behavioral changes. Name and FullyQualifiedName currently both return just the stack name. In a future change, once project name is incorporated into the object, FullyQualifiedName will be able to return `organization/$project/$name`. Change extracted from #12134 Co-authored-by: Fraser Waters <fraser@pulumi.com>

bors · 2023-03-15T08:14:56Z

Timed out.

filestate backend currently operates exclusively with stack names. All its internal pass around just the stack name, and nothing else. This makes it a bit difficult to add project support to the backend. This is a refactor in advance of adding project support, changing the internals of filestate to pass a stack reference around. It inspects the reference directly for all its operations. Note: This contains no behavioral changes. Name and FullyQualifiedName currently both return just the stack name. In a future change, once project name is incorporated into the object, FullyQualifiedName will be able to return `organization/$project/$name`. Extracted from #12134 Co-authored-by: Abhinav Gupta <abhinav@pulumi.com>

abhinav · 2023-03-16T22:16:00Z

bors merge

bors · 2023-03-16T22:44:58Z

Build succeeded:

bors-ok

This was referenced Mar 13, 2023

sdk/common: Add testing/diagtest package #12424

Merged

WIP: Readd project support to filestate #12134

Closed

abhinav mentioned this pull request Mar 13, 2023

filestate: Plumb context through constructor #12425

Merged

abhinav added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Mar 13, 2023

Frassle reviewed Mar 13, 2023

View reviewed changes

Base automatically changed from abhinav/filestate-ctx to master March 13, 2023 23:10

abhinav force-pushed the abhinav/filestate-use-local-ref branch from d0d9d8a to 9eedec4 Compare March 14, 2023 00:32

Frassle reviewed Mar 14, 2023

View reviewed changes

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

abhinav force-pushed the abhinav/filestate-use-local-ref branch from 9eedec4 to 14fd8f9 Compare March 14, 2023 16:27

Frassle approved these changes Mar 14, 2023

View reviewed changes

This was referenced Mar 15, 2023

filestate: Introduce referenceStore to control layout #12436

Merged

backend/filestate: Re-add project support #12437

Merged

{cli, filestate}: Add 'state upgrade' command #12438

Merged

abhinav force-pushed the abhinav/filestate-use-local-ref branch from 14fd8f9 to 5ba5f26 Compare March 16, 2023 21:45

bors bot merged commit c05e46c into master Mar 16, 2023

bors bot deleted the abhinav/filestate-use-local-ref branch March 16, 2023 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filestate/internal: Use stack reference, not name #12426

filestate/internal: Use stack reference, not name #12426

abhinav commented Mar 13, 2023

abhinav commented Mar 13, 2023 •

edited

pulumi-bot commented Mar 13, 2023 •

edited

Frassle left a comment

Frassle Mar 13, 2023

abhinav Mar 13, 2023

Frassle Mar 14, 2023

abhinav Mar 14, 2023

Frassle Mar 14, 2023

abhinav Mar 14, 2023

abhinav Mar 14, 2023

abhinav commented Mar 15, 2023

bors bot commented Mar 15, 2023

bors bot commented Mar 15, 2023

abhinav commented Mar 16, 2023

bors bot commented Mar 16, 2023

filestate/internal: Use stack reference, not name #12426

filestate/internal: Use stack reference, not name #12426

Conversation

abhinav commented Mar 13, 2023

abhinav commented Mar 13, 2023 • edited

pulumi-bot commented Mar 13, 2023 • edited

Changelog

[uncommitted] (2023-03-16)

Frassle left a comment

Choose a reason for hiding this comment

Frassle Mar 13, 2023

Choose a reason for hiding this comment

abhinav Mar 13, 2023

Choose a reason for hiding this comment

Frassle Mar 14, 2023

Choose a reason for hiding this comment

abhinav Mar 14, 2023

Choose a reason for hiding this comment

Frassle Mar 14, 2023

Choose a reason for hiding this comment

abhinav Mar 14, 2023

Choose a reason for hiding this comment

abhinav Mar 14, 2023

Choose a reason for hiding this comment

abhinav commented Mar 15, 2023

bors bot commented Mar 15, 2023

bors bot commented Mar 15, 2023

abhinav commented Mar 16, 2023

bors bot commented Mar 16, 2023

abhinav commented Mar 13, 2023 •

edited

pulumi-bot commented Mar 13, 2023 •

edited