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

fix: add fail index to streaming sequence #1364

Merged
merged 7 commits into from
Aug 14, 2023
Merged

Conversation

leahecole
Copy link
Contributor

@leahecole leahecole commented Aug 10, 2023

Gal was working on this before he moved to another team and I am opening this to make sure I tie up loose ends by finishing his work. I added a test and then I regenerated the support files because of the proto change. It seemed to regenerate more than just my service, but I removed anything unrelated to sequence. If I should re-include those, let me know. cc @galz10

@leahecole leahecole changed the title Sequence streaming fix fix: add fail index to streaming sequence Aug 10, 2023
@leahecole leahecole marked this pull request as ready for review August 11, 2023 19:17
@leahecole leahecole requested review from a team as code owners August 11, 2023 19:17
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

I don't think you need to include the generated code changes in this PR. They should be automatically included in CI for testing, then contributed back via an automatic PR upon merge.



int32 fail_index = 2 [
(google.api.resource_reference).type = "showcase.googleapis.com/Sequence",
Copy link
Contributor

@blakeli0 blakeli0 Aug 11, 2023

Choose a reason for hiding this comment

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

@noahdietz Is this a valid resource reference? I think the field reference another resource should be of String type per AIP-122, also the Sequence resource is of pattern sequences/{sequence} which seems do not fit here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah definitely not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! I accepted Noah's suggestion which iiuc just removed what used to be line 30 with this reference type. I added a clarifying comment, good call @blakeli0. I want to make sure I'm interpreting these comments and the code suggestion from @noahdietz correctly though - do I not need to include a resource type at all? Or, should I be linking to something like

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, not necessary. The resource here is the Sequence being attempted. The content is just data.

schema/google/showcase/v1beta1/sequence.proto Outdated Show resolved Hide resolved
@leahecole leahecole merged commit 567e756 into main Aug 14, 2023
11 checks passed
@leahecole leahecole deleted the sequence-streaming-fix branch August 14, 2023 16:57
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.

4 participants