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

[engine] Add support for source positions #13437

Merged
merged 1 commit into from
Jul 11, 2023
Merged

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Jul 6, 2023

These changes add support for passing source position information in gRPC metadata and recording the source position that corresponds to a resource registration in the statefile.

Enabling source position information in the resource model can provide substantial benefits, including but not limited to:

  • Better errors from the Pulumi CLI
  • Go-to-defintion for resources in state
  • Editor integration for errors, etc. from pulumi preview

Source positions are (file, line) or (file, line, column) tuples represented as URIs. The line and column are stored in the fragment portion of the URI as "line(,column)?". The scheme of the URI and the form of its path component depends on the context in which it is generated or used:

  • During an active update, the URI's scheme is file and paths are absolute filesystem paths. This allows consumers to easily access arbitrary files that are available on the host.
  • In a statefile, the URI's scheme is project and paths are relative to the project root. This allows consumers to resolve source positions relative to the project file in different contexts irrespective of the location of the project itself (e.g. given a project-relative path and the URL of the project's root on GitHub, one can build a GitHub URL for the source position).

During an update, source position information may be attached to gRPC calls as "source-position" metadata. This allows arbitrary calls to be associated with source positions without changes to their protobuf payloads. Modifying the protobuf payloads is also a viable approach, but is somewhat more invasive than attaching metadata, and requires changes to every call signature.

Source positions should reflect the position in user code that initiated a resource model operation (e.g. the source position passed with RegisterResource for pet in the example above should be the source position in index.ts, not the source position in the Pulumi SDK). In general, the Pulumi SDK should be able to infer the source position of the resource registration, as the relationship between a resource registration and its corresponding user code should be static per SDK.

Source positions in state files will be stored as a new registeredAt property on each resource. This property is optional.

Part of #13436

@pgavlin pgavlin added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jul 6, 2023
@pgavlin pgavlin requested a review from justinvp July 6, 2023 23:13
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Jul 6, 2023

Changelog

[uncommitted] (2023-07-10)

@pgavlin
Copy link
Member Author

pgavlin commented Jul 6, 2023

I should point out that these changes don't add any code to the various language SDKs to start reporting source positions. I'll submit additional PRs with those changes.

@pgavlin
Copy link
Member Author

pgavlin commented Jul 7, 2023

(implementations for the core SDKs are in this branch: pgavlin/source-position...pgavlin/source-position-sdks; only the Node implementation has been tested)

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

Have we given any consideration to how MLCs are going to work with this?
Feels like they probably need their positions stripped by the engine, or somehow marked that their a remote source, not the project source.

pkg/resource/deploy/deploytest/resourcemonitor.go Outdated Show resolved Hide resolved
pkg/resource/deploy/source_eval.go Show resolved Hide resolved
pkg/resource/deploy/source_eval.go Outdated Show resolved Hide resolved
pkg/resource/deploy/source_eval.go Outdated Show resolved Hide resolved
@Frassle
Copy link
Member

Frassle commented Jul 7, 2023

Modifying the protobuf payloads is also a viable approach, but is somewhat more invasive than attaching metadata, and requires changes to every call signature.

I think I'd prefer that. This doesn't really feel like metadata given how used it's going to end up being. It also makes it a lot easier for us to document for anyone implementing their own language plugins.

@pgavlin
Copy link
Member Author

pgavlin commented Jul 7, 2023

Have we given any consideration to how MLCs are going to work with this?
Feels like they probably need their positions stripped by the engine, or somehow marked that their a remote source, not the project source.

Yes--and packages more generally. Part of the motivation behind using URIs rather than simple paths is s.t. we can support alternative schemes. I imagine that one of those schemes might be package://package-name/path#line,column.

Modifying the protobuf payloads is also a viable approach, but is somewhat more invasive than attaching metadata, and requires changes to every call signature.

I think I'd prefer that. This doesn't really feel like metadata given how used it's going to end up being. It also makes it a lot easier for us to document for anyone implementing their own language plugins.

That sounds okay to me. It's a bit of a drag to add this field to registerresource, readresource, invoke, streaminvoke, call, and perhaps registerresourceoutputs, but ultimately it's not too bad.

pkg/resource/deploy/source_eval.go Show resolved Hide resolved
pkg/resource/deploy/source_eval.go Show resolved Hide resolved
These changes add support for passing source position information in
gRPC metadata and recording the source position that corresponds to a
resource registration in the statefile.

Enabling source position information in the resource model can provide
substantial benefits, including but not limited to:

- Better errors from the Pulumi CLI
- Go-to-defintion for resources in state
- Editor integration for errors, etc. from `pulumi preview`

Source positions are (file, line) or (file, line, column) tuples
represented as URIs. The line and column are stored in the fragment
portion of the URI as "line(,column)?". The scheme of the URI and the
form of its path component depends on the context in which it is
generated or used:

- During an active update, the URI's scheme is `file` and paths are
  absolute filesystem paths. This allows consumers to easily access
  arbitrary files that are available on the host.
- In a statefile, the URI's scheme is `project` and paths are relative
  to the project root. This allows consumers to resolve source positions
  relative to the project file in different contexts irrespective of the
  location of the project itself (e.g. given a project-relative path and
  the URL of the project's root on GitHub, one can build a GitHub URL for
  the source position).

During an update, source position information may be attached to gRPC
calls as "source-position" metadata. This allows arbitrary calls to be
associated with source positions without changes to their protobuf
payloads. Modifying the protobuf payloads is also a viable approach, but
is somewhat more invasive than attaching metadata, and requires changes
to every call signature.

Source positions should reflect the position in user code that initiated
a resource model operation (e.g. the source position passed with
`RegisterResource` for `pet` in the example above should be the source
position in `index.ts`, _not_ the source position in the Pulumi SDK). In
general, the Pulumi SDK should be able to infer the source position of
the resource registration, as the relationship between a resource
registration and its corresponding user code should be static per SDK.

Source positions in state files will be stored as a new `registeredAt`
property on each resource. This property is optional.
@pgavlin
Copy link
Member Author

pgavlin commented Jul 11, 2023

bors merge

bors bot added a commit that referenced this pull request Jul 11, 2023
13437: [engine] Add support for source positions r=pgavlin a=pgavlin

These changes add support for passing source position information in gRPC metadata and recording the source position that corresponds to a resource registration in the statefile.

Enabling source position information in the resource model can provide substantial benefits, including but not limited to:

- Better errors from the Pulumi CLI
- Go-to-defintion for resources in state
- Editor integration for errors, etc. from `pulumi preview`

Source positions are (file, line) or (file, line, column) tuples represented as URIs. The line and column are stored in the fragment portion of the URI as "line(,column)?". The scheme of the URI and the form of its path component depends on the context in which it is generated or used:

- During an active update, the URI's scheme is `file` and paths are absolute filesystem paths. This allows consumers to easily access arbitrary files that are available on the host.
- In a statefile, the URI's scheme is `project` and paths are relative to the project root. This allows consumers to resolve source positions relative to the project file in different contexts irrespective of the location of the project itself (e.g. given a project-relative path and the URL of the project's root on GitHub, one can build a GitHub URL for the source position).

During an update, source position information may be attached to gRPC calls as "source-position" metadata. This allows arbitrary calls to be associated with source positions without changes to their protobuf payloads. Modifying the protobuf payloads is also a viable approach, but is somewhat more invasive than attaching metadata, and requires changes to every call signature.

Source positions should reflect the position in user code that initiated a resource model operation (e.g. the source position passed with `RegisterResource` for `pet` in the example above should be the source position in `index.ts`, _not_ the source position in the Pulumi SDK). In general, the Pulumi SDK should be able to infer the source position of the resource registration, as the relationship between a resource registration and its corresponding user code should be static per SDK.

Source positions in state files will be stored as a new `registeredAt` property on each resource. This property is optional.

Part of #13436

Co-authored-by: Pat Gavlin <pat@pulumi.com>
@bors
Copy link
Contributor

bors bot commented Jul 11, 2023

Build failed:

@pgavlin
Copy link
Member Author

pgavlin commented Jul 11, 2023

bors retry

@bors
Copy link
Contributor

bors bot commented Jul 11, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit fc8ca6f into master Jul 11, 2023
50 checks passed
@bors bors bot deleted the pgavlin/source-position branch July 11, 2023 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants