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

support rotating by path #437

Merged
merged 7 commits into from
Jan 30, 2025
Merged

support rotating by path #437

merged 7 commits into from
Jan 30, 2025

Conversation

nyobe
Copy link
Contributor

@nyobe nyobe commented Jan 27, 2025

Re-add support for specifying a subset of paths to rotate to #432.

Closes https://github.com/pulumi/pulumi-service/issues/25443

@nyobe nyobe added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jan 27, 2025
@nyobe nyobe marked this pull request as ready for review January 27, 2025 21:02
@@ -1 +1 @@
{"rotate": true}
{"rotate": true, "rotatePaths": ["full", "short"]}
Copy link
Member

Choose a reason for hiding this comment

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

would love to see some more complex paths here--e.g. with quoted["property \"accessors\""], indices into lists, and normal dotted property access

Copy link
Contributor Author

@nyobe nyobe Jan 27, 2025

Choose a reason for hiding this comment

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

I've added more complex paths, and combined with parsing property paths it works well, thank you.

However, I tried to include one that is embedded within another function, but it seems like the expr paths in the evaluator are getting reset when parsing function arguments. should that be expected to work? (eg the expr's path when evaluating the examples.embedded-in-another-fn["fn::open::test"].some-input is just some-input)

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm interesting. That's worth some more digging.

So there are two sorts of paths at play here: value paths and document paths. The former are the paths used in ${symbols.and.interpolations} and are resolved with respect to the schema of the fully-evaluated environment. The latter are resolved with respect to the YAML document. We want these paths to be document paths. I'm not sure what sort the paths on the expr nodes are (or what they're intended to be).

Copy link
Contributor Author

@nyobe nyobe Jan 28, 2025

Choose a reason for hiding this comment

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

okay, I'm not 100% sure, but I think that maybe expr.path is might be intended to be value path, based on the comment that it's for reporting cyclic reference errors. The path is built up by declare, which does reset the path when recursing into fn arguments.. (I think this might not be quite right even for value paths, since they should still be qualified by the parent path, but just eliding the function name, right?)

Anyway, it looks like I might be able to recover the document path from the ast node, using x.repr.syntax().Syntax().Syntax().Path() (which resolves to expr -> exprRepr -> ast.Expr -> syntax.Node -> syntax.Syntax (encoding.YAMLSyntax))`, so I'll switch to this instead.

@nyobe nyobe force-pushed the claire/rotate-by-path branch 2 times, most recently from 4aec74a to 18688ec Compare January 27, 2025 22:38
@nyobe nyobe force-pushed the claire/rotate-by-path branch from 18688ec to bac0a47 Compare January 27, 2025 22:43
@nyobe nyobe force-pushed the claire/rotate-by-path branch from dd8d1e6 to 8c1de5a Compare January 28, 2025 01:25
@nyobe nyobe requested a review from pgavlin January 29, 2025 00:42
Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

One note about a surprising test case, LGTM otherwise

@nyobe nyobe enabled auto-merge (squash) January 30, 2025 03:06
@nyobe nyobe merged commit 0f679a7 into main Jan 30, 2025
6 checks passed
@nyobe nyobe deleted the claire/rotate-by-path branch January 30, 2025 03:07
nyobe added a commit that referenced this pull request Feb 7, 2025
Extends #432 with support for specifying a subset of paths to rotate.

Closes pulumi/pulumi-service#24986
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.

3 participants