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

Clarify that delegations are optional #157

Closed

Conversation

MVrachev
Copy link
Contributor

Nowhere in the spec, we clarify that "delegations" is an optional field
in the targets metadata file.
This is a possible reason why (at the time of writing this commit) in
the TUF python reference implementation "delegations" is still a
required field.

Signed-off-by: Martin Vrachev [email protected]

Nowhere in the spec, we clarify that "delegations" is an optional field
in the targets metadata file.
This is a possible reason why (at the time of writing this commit) in
the TUF python reference implementation "delegations" is still a
required field.

Signed-off-by: Martin Vrachev <[email protected]>
Copy link
Collaborator

@mnm678 mnm678 left a comment

Choose a reason for hiding this comment

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

Great clarification. I believe this preserves backwards compatibility (required -> optional), so LGTM.

@jku
Copy link
Member

jku commented Apr 22, 2021

the TUF python reference implementation "delegations" is still a required field.

What is this based on btw (where is it required)?

@MVrachev
Copy link
Contributor Author

@jku
Copy link
Member

jku commented Apr 22, 2021

Ah right: so it's a bug in new API. Fixing that definitely does not preserve backwards compatibility (a client that assumed delegations are required by using the current API will break if the server now makes delegations optional) but that seems ok if it's in api/metadata.py...

In any case that's an implementation issue: the spec is not changing its position, it's clarifying an existing one as I see it: the definition of "signed" already has delegations in parentheses (the use of parenthesis in the spec does not seem entirely consistent but in this case I read that as meaning "optional") . LGTM.

MVrachev added a commit to MVrachev/tuf that referenced this pull request Apr 28, 2021
According to the spec, delegations in targets are marked as optional:
https://theupdateframework.github.io/specification/latest/#file-formats-targets
and a pr, clarifying that even more, is approved:
theupdateframework/specification#157.

This is a possible breaking change.

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/tuf that referenced this pull request Apr 29, 2021
According to the spec, delegations in targets are marked as optional:
https://theupdateframework.github.io/specification/latest/#file-formats-targets
and a pr, clarifying that even more, is approved:
theupdateframework/specification#157.

This is a possible breaking change.

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/tuf that referenced this pull request Apr 29, 2021
According to the spec, delegations in targets are marked as optional:
https://theupdateframework.github.io/specification/latest/#file-formats-targets
and a pr, clarifying that even more, is approved:
theupdateframework/specification#157.

This is a possible breaking change.

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/tuf that referenced this pull request May 10, 2021
According to the spec, delegations in targets are marked as optional:
https://theupdateframework.github.io/specification/latest/#file-formats-targets
and a pr, clarifying that even more, is approved:
theupdateframework/specification#157.

This is a possible breaking change.

Signed-off-by: Martin Vrachev <[email protected]>
@joshuagl
Copy link
Member

joshuagl commented May 26, 2021

Per the scope section of the spec we use keywords from RFC 2119, so we should say OPTIONAL in uppercase.

The custom field is also optional, could you add a change to address that also?

@MVrachev
Copy link
Contributor Author

Superseded by #165.

@MVrachev MVrachev closed this May 31, 2021
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