Skip to content
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

Send smart aliases via gRPC to engine #9731

Merged
merged 20 commits into from Jun 16, 2022
Merged

Send smart aliases via gRPC to engine #9731

merged 20 commits into from Jun 16, 2022

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented May 30, 2022

Description

This add an new field on the RegisterResource and ReadResource protobuf messages to the resource monitor. As well as the repeated string aliases field, we now have a repeated Alias smartAliases field (name open to scrutiny). The Alias object is a protobuf encoding to match the alias objects we have in the SDKs where users can just set "name", or "type". Those objects worked by the SDK then resolving down to a URN, that resolution logic is now also in the engine and so eventually we'll be able to remove all that logic from every SDK (we can't remove it straight away because of back compatibility, a user may have a new SDK but old engine).

Fixes #9734

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@Frassle
Copy link
Member Author

Frassle commented Jun 14, 2022

/run-integration-tests

@github-actions
Copy link

Please view the results of the PR Build + Acceptance Tests Run Here

@Frassle
Copy link
Member Author

Frassle commented Jun 14, 2022

/run-integration-tests

@github-actions
Copy link

Please view the results of the PR Build + Integration Tests Run Here

@Frassle Frassle marked this pull request as ready for review June 15, 2022 09:22
@AaronFriel
Copy link
Member

AaronFriel commented Jun 15, 2022

Curious how big of a change in the engine would it be to rename the existing field (keeping tag ID the same) to alias_urns and naming the new field aliases? I believe protobufs allow these sort of renames as the thing that matters are the type and tag numbers?

I'd then add a [deprecated = true] to the old field, which is a no-op in most languages but might add an annotation or IDE hint.

@Frassle
Copy link
Member Author

Frassle commented Jun 15, 2022

Curious how big of a change in the engine would it be to rename the existing field (keeping tag ID the same) to alias_urns and naming the new field aliases? I believe protobufs allow these sort of renames as the thing that matters are the type and tag numbers?

I'd then add a [deprecated = true] to the old field, which is a no-op in most languages but might add an annotation or IDE hint.

Done. Interestingly a discovery fell out of that rename that we didn't look at aliases for resource reads, and only the Go SDK tried to send them. I'm not sure if aliases make sense for reads anyway so I've just removed that field for now.

Would be good to get @pgavlin check on if alias for reads is/isn't needed.

Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment is that this makes me long for codegen-ing resource options. It's a lot of little changes to review in each SDK.

Looking forward to deleting "inheritedChildAlias" and all the other machinery in the SDKs in 4.0.

@@ -104,6 +104,7 @@ type ResourceOptions struct {
Remote bool
Providers map[string]string
AdditionalSecretOutputs []resource.PropertyKey
SmartAliases []resource.Alias
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Able to rename this field as well?

@Frassle
Copy link
Member Author

Frassle commented Jun 16, 2022

My only comment is that this makes me long for codegen-ing resource options. It's a lot of little changes to review in each SDK.

Working on a design for that today 😄

Able to rename this field as well?

Yes will do!

@AaronFriel
Copy link
Member

LGTM

@Frassle Frassle merged commit cba1880 into master Jun 16, 2022
@pulumi-bot pulumi-bot deleted the fraser/engineAlias branch June 16, 2022 21:07
Frassle added a commit that referenced this pull request Jun 29, 2022
Frassle added a commit that referenced this pull request Jun 29, 2022
* Revert "Send smart aliases via gRPC to engine (#9731)"

This reverts commit cba1880.

* Just fully revert the protobuf changes

* Add to CHANGELOG
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
* Send smart aliases via gRPC to engine

* Add to SupportsFeature

* Restore old logic when the engine doesn't support smartAliases

* Add to deploytest ResourceOptions

* Add tests

* Add to CHANGELOG

* Fix test

* Rename proto fields

* Regenerate protobufs

* Fix up SDKs after field rename

* Rename deploytest aliases

* Rename internal fields

* Fix typo in c# code

* Fix typescript

* Rename feature to aliasSpecs

* Rename type to Spec
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
…ulumi/pulumi#9999)

* Revert "Send smart aliases via gRPC to engine (pulumi/pulumi#9731)"

This reverts commit c58062b.

* Just fully revert the protobuf changes

* Add to CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add alias support to the engine
2 participants