-
Notifications
You must be signed in to change notification settings - Fork 86
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
Move VMMetadata proto to a more fitting home #6341
Conversation
0ca4ee0
to
ab9bdb1
Compare
ab9bdb1
to
d8cd7c6
Compare
8ccbd4b
to
5a6a5a7
Compare
5a6a5a7
to
5c1109b
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.
nice, thanks for making this change!
return this.getDeprecatedFirecrackerVMMetadata(); | ||
} | ||
for (const metadata of auxiliaryMetadata) { | ||
if (metadata.typeUrl.includes("firecracker.VMMetadata")) { |
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.
nit: can we do an equality check on the typeUrl
instead of .includes()
? or does the URL include some contents we aren't expecting?
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.
Changed. I thought the includes might be slightly less brittle, but don't feel strongly
The VMMetadata proto feels a little too firecracker-specific to live in remote_execution.proto. I've hit some problems with import cycles (firecracker.proto needs to import remote_execution.proto for generic RBE concepts like
files
, but now there is an implicit import loop because remote_execution.proto contains the definition for this firecracker-specific field).This PR sets VMMetadata in the more generic
repeated google.protobuf.Any auxiliary_metadata
field ofExecutedActionMetadata
. That allows us to move the proto definition into firecracker.protoRelated issues: Based on feedback/challenges from #6303