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: Ensure event annotations is included in compile-time protobuf descriptors #1945

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented Dec 3, 2024

A FileDescriptor that is a compile-time dependency is not considered the same as the FileDescriptor loaded at run-time from a FileDescriptorSet for the same file. This ensures that the FileDescriptor for event_annotations.proto known at compile-time is used instead of one that's possibly loaded from the --event-message-descriptor-set option.

@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas
Copy link
Member Author

SanjayVas commented Dec 3, 2024

This is intended to resolve #1934. For reasons discussed offline, there is no good way to test this in the Halo codebase without significant refactoring to ensure that the event message type used for the test is not a compile-time dependency1. Therefore testing/verification is reliant on operators that use this feature, e.g. Origin.

Footnotes

  1. More accurately, that no build target that includes the EDP simulator also transitively includes the event and/or metadata types used for testing

@SanjayVas SanjayVas force-pushed the sanjayvas-simulator-templates branch from 359c4ab to 1865986 Compare December 3, 2024 00:23
@SanjayVas SanjayVas force-pushed the sanjayvas-simulator-templates branch from 1865986 to 7e534c6 Compare December 3, 2024 00:37
Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas merged commit e9856aa into main Dec 3, 2024
5 of 9 checks passed
@SanjayVas SanjayVas deleted the sanjayvas-simulator-templates branch December 3, 2024 05:05
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.

3 participants