-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
@@ -1 +1 @@ | |||
{"rotate": true} | |||
{"rotate": true, "rotatePaths": ["full", "short"]} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
4aec74a
to
18688ec
Compare
18688ec
to
bac0a47
Compare
dd8d1e6
to
8c1de5a
Compare
There was a problem hiding this 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
Extends #432 with support for specifying a subset of paths to rotate. Closes pulumi/pulumi-service#24986
Re-add support for specifying a subset of paths to rotate to #432.
Closes https://github.com/pulumi/pulumi-service/issues/25443