Skip to content

Conversation

nyobe
Copy link
Contributor

@nyobe nyobe commented Mar 28, 2025

As part of adding the retry-rotation button to the console, I realized that the rotation paths that we output as part of rotation results cannot be directly used as rotation path inputs. This is because rotation path inputs get implicitly prefixed by values., while the result paths are physical docpaths that already have the values prefix.

Right now the frontend works around this by strip the values.? prefix from the result paths, but this just exposes the fact that the prefixing doesn't actually work correctly for quoted keys like ["rotated-creds"] -> values.["rotated-creds"] -> invalid path

I think that doing this prefixing in eval.RotateEnvironment is probably a mistake. It was done to try to match the behavior of the paths used by the CLI. So, while this is technically a breaking change, I propose moving the prefixing behavior to the CLI instead, and have the eval apis just use physical docpaths consistently.

@nyobe nyobe requested review from pgavlin and seanyeh March 28, 2025 22:50
Copy link
Contributor

@IaroslavTitov IaroslavTitov left a comment

Choose a reason for hiding this comment

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

Would this break anything in pulumi-service? Probably have to manually update to the new version and fix up where you add values. in the console?

@nyobe
Copy link
Contributor Author

nyobe commented Apr 4, 2025

Would this break anything in pulumi-service? Probably have to manually update to the new version and fix up where you add values. in the console?

yeah, we'll need to update the frontend at the same time as bumping the ESC version to back out this workaround

I don't believe we're inputting rotation paths anywhere else. The biggest concern I have is that the rotate command in the current version of the CLI won't send the paths with the values prefix to the API, so it won't work correctly. But I suppose we could add a check in the rotate api handler to see if paths are prefixed or not, and try to fix them up if not.

@IaroslavTitov
Copy link
Contributor

IaroslavTitov commented Apr 9, 2025

I suppose we could add a check in the rotate api handler to see if paths are prefixed or not, and try to fix them up if not.

Yeah, def need to make sure this doesn't break any of the callers, like frontend, CLI, ESC SDK. The frontend you can change at the same time easily, and if it doesn't break others, should be fine to just go in?

@nyobe nyobe self-assigned this May 19, 2025
@nyobe nyobe added this to the 0.121 milestone May 19, 2025
@nyobe nyobe modified the milestones: 0.121, 0.122 Jun 9, 2025
@komalali komalali removed this from the 0.122 milestone Oct 14, 2025
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.

5 participants