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

Update identity related fields in protocol move request #36662

Merged
merged 4 commits into from
Mar 10, 2025

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Mar 10, 2025

This PR addresses two shortcomings when dealing with identities in the context of cross resource moves.

  1. It adds the source_identity_schema_version fields to include all information a provider may need to decode the data
  2. It changes the type of the source_identity to make it more clear that it contains the raw identity data

Target Release

1.12.x

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dbanck dbanck added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Mar 10, 2025
@dbanck dbanck requested a review from a team as a code owner March 10, 2025 13:39
jbardin
jbardin previously approved these changes Mar 10, 2025
Comment on lines +680 to +683
// The raw identity of the resource being moved. Only the json field is
// populated, as there should be no legacy providers using the flatmap
// format that support newly introduced RPCs.
RawState source_identity = 7;
Copy link
Member

Choose a reason for hiding this comment

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

Should we be consistent with UpgradeResourceIdentity here and just use bytes?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe UpgradeResourceIdentity should be consistent with the existing calls and use RawState?

Copy link
Member

Choose a reason for hiding this comment

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

Either/or 😆

I actually dropped a comment on the RFC recently about potential "future" data in identity and whether that would potentially be part of these types of RPCs: https://docs.google.com/document/d/1jvNfineZgXaKFP5djLkMxXKf9s6Xq-FzBV3p2jHGbiI/edit?disco=AAABe3pkd1Y

If that seems like something we might do, perhaps a new RawIdentity would fit as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I'll update it the other way around to be consistent.

The idea there was that if we ever added a new base encoding that wasn't json, it could be added to that message type.

Copy link
Member

Choose a reason for hiding this comment

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

RawState is just meant to represent raw encoded data from the state file, and not imply any meaning of that data. The encoded identity is just a chunk of the resource's state, with the same encoding as the state in general, hence RawState. In reality RawState was required at the time of the 0.11-0.12 transition because there were multiple state encodings, and it's also been maintained since because however unlikely there could a different encoding in the future. We could decide that all raw state encoded data must be json, but I think that's outside the scope here, and we should use the existing patterns in place until then ;)

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, sounds good to me 👍🏻

@dbanck dbanck merged commit 5b48de5 into main Mar 10, 2025
8 checks passed
@dbanck dbanck deleted the dbanck/update-move-protocol branch March 10, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog-needed Add this to your PR if the change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants