-
Notifications
You must be signed in to change notification settings - Fork 110
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(data): add back SnapshotFileMeta.Custom #373
fix(data): add back SnapshotFileMeta.Custom #373
Conversation
93b9650
to
743472d
Compare
I'm not strictly against this PR, but a few things:
|
Thanks @arbll! As with @trishankatdatadog, this field is not in the TUF specification: https://theupdateframework.github.io/specification/latest/#file-formats-snapshot If you want to keep information about a delegated target, then ideally the information should live in/signed by the parent role, rather than the snapshot role, correct? It seems like you may want CUSTOM metadata inside a I'm on the side of adding this specifically to |
Agreed. Is there a good way to make this optional and not built or at least included in the metadata (even as an empty field) by default? |
743472d
to
52f27c3
Compare
I also want to make sure that we only need this kind of surgical addition, and not have additional PRs later adding this in other places. |
Should be omitted when empty! So I think this is good. |
I am worried about end-users getting confused about this custom addition, so we need to at least document why it's there. |
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.
Approval, but to accomodate, let's add a commend above type SnapshotFileMeta
that it includes Custom
.
Don't think end-users will even see the field if it isn't populated, but worth a comment
@arbll pls make the change? thx! |
52f27c3
to
fda5485
Compare
Thanks @arbll. Would you please:
|
4076a7f
to
85b22ad
Compare
Signed-off-by: Arthur Bellal <arthur.bellal@datadoghq.com>
85b22ad
to
0518f2f
Compare
@trishankatdatadog Done |
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.
I'm not crazy about this PR (we should add a deprecation notice and figure out alternatives), but happy to unblock for now.
add back SnapshotFileMeta.custom Signed-off-by: Arthur Bellal <arthur.bellal@datadoghq.com> Signed-off-by: Arthur Bellal <arthur.bellal@datadoghq.com>
Adds back a
Custom
field toSnapshotFileMeta
following its removal in #345.Release Notes: Add back a
Custom
field toSnapshotFileMeta
Types of changes:
Description of the changes being introduced by the pull request:
We rely on this
Custom
field to store some custom data related to delegatedtargets
. The removal of this field would force us to store this outside of the signed TUF file or in a hacky map inSnapshot.Custom
.Please verify and check that the pull request fulfills the following requirements: