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.

Sign up for GitHub

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

Jump to bottom

content: schema are informative, spec text is canonical #892

Merged
merged 3 commits into from
Aug 24, 2023
Merged

content: schema are informative, spec text is canonical #892

merged 3 commits into from
Aug 24, 2023

Conversation

joshuagl
Copy link
Member

As discussed in the 2023-06-05 spec meeting clarify the format of ResourceDescriptor and make clear that the schema are only informative, with the specification text (whether our text or the linked in-toto/attestation spec) being the canonical definition.

  • Update protobuf and cue definitions to match the in-toto attestations specification.
  • Emphasise that the specification text, not the schema, is the authoritative specification should they be in conflict.
  • Link to an immutable version of the in-toto specification to avoid potential changes in in-toto resulting in SemVer breaking changes to SLSA's formats.

Fixes: #875

@netlify
Copy link

netlify bot commented Jun 29, 2023 •
edited
Loading

✅ Deploy Preview for slsa ready!

Name Link
🔨 Latest commit 04fe7d7
🔍 Latest deploy log https://app.netlify.com/sites/slsa/deploys/64e724557100d900080e1865
😎 Deploy Preview https://deploy-preview-892--slsa.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@joshuagl joshuagl mentioned this pull request Jun 29, 2023
Closed
MarkLodato
MarkLodato reviewed Jun 29, 2023
View reviewed changes
Copy link
Member

@MarkLodato MarkLodato left a comment

Choose a reason for hiding this comment

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

Thanks! I just have a suggestion on clarity

@joshuagl joshuagl changed the title Clarify that schema are informative and specification text canonical spec: clarify that schema are informative, spec text is canonical Jun 29, 2023
@joshuagl joshuagl changed the title spec: clarify that schema are informative, spec text is canonical spec: schema are informative, spec text is canonical Jun 29, 2023
joshuagl
joshuagl commented Jun 30, 2023
View reviewed changes
MarkLodato
MarkLodato approved these changes Jul 1, 2023
View reviewed changes
@MarkLodato
Copy link
Member

Is this an editorial or a spec change? (Affects how # of reviews and waiting period.)

@joshuagl
Copy link
Member Author

joshuagl commented Jul 3, 2023 •
edited
Loading

Is this an editorial or a spec change? (Affects how # of reviews and waiting period.)

Great question. I think it's worth handling as spec-content, so that we can be sure of consensus and approval. The specification does not change in content IFF you agree with these changes (that the text are authoritative), so it's worth being sure that 2 other approvers agree :-D

@joshuagl joshuagl changed the title spec: schema are informative, spec text is canonical spec-content: schema are informative, spec text is canonical Jul 3, 2023
@MarkLodato MarkLodato requested a review from lehors July 5, 2023 12:51
@kpk47 kpk47 mentioned this pull request Jul 6, 2023
Open
7 tasks
lehors
lehors approved these changes Jul 10, 2023
View reviewed changes
Copy link
Member

@lehors lehors left a comment

Choose a reason for hiding this comment

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

LGTM but so we aren't changing the version number at all. Should there be a new entry added to the change history or a note somewhere? This does seem worth some notification to the reader somehow.

@MarkLodato MarkLodato changed the title spec-content: schema are informative, spec text is canonical content: schema are informative, spec text is canonical Jul 10, 2023
@MarkLodato
Copy link
Member

LGTM but so we aren't changing the version number at all. Should there be a new entry added to the change history or a note somewhere? This does seem worth some notification to the reader somehow.

Good point. How about we require a changelog entry for every content change?

If so, then might want to consider every content change a semver "patch" release, which would only show up in the changelog?

@lehors
Copy link
Member

lehors commented Jul 10, 2023

Yes, I think having a patch release would make sense.

@joshuagl
Copy link
Member Author

I like the idea of rolling a patch release for this (and a few other in-flight changes, see #900).

Are we treating the what's new? page as the changelog, with a new version of the page for each release?

@lehors
Copy link
Member

lehors commented Jul 10, 2023

I think that would make sense.

@MarkLodato
Copy link
Member

Friendly ping. Is this going in v1.0 or v1.1? The latter now exists as of #942 FYI.

@joshuagl joshuagl force-pushed the joshuagl/annotations branch from c1e8018 to 87b5c51 Compare August 21, 2023 09:29
@joshuagl
Copy link
Member Author

Consensus was for this to target v1.1. I've updated the PR to make the changes against the v1.1 folder.

@joshuagl
Copy link
Member Author

Reviewing the conversation here, I still need to update "what's new" for this content change

joshuagl and others added 2 commits August 23, 2023 10:01
@joshuagl
spec: make it clear text is authorative
9baf680 
The text are the authorative specification, the schema (protobuf and
cue) are intended to be pedagogical aids but do not invalidate or
supercede what is written in the specification text.

Signed-off-by: Joshua Lock <joshua.lock@uk.verizon.com>
@joshuagl
Update what's new page for this content change
4433f74 
Signed-off-by: Joshua Lock <joshuagloe@gmail.com>
@joshuagl joshuagl force-pushed the joshuagl/annotations branch from 87b5c51 to 4433f74 Compare August 23, 2023 09:02
@joshuagl
Copy link
Member Author

I've added a very minimal "What's new" page for v1.1 which describes this change: https://deploy-preview-892--slsa.netlify.app/spec/v1.1/whats-new

MarkLodato
MarkLodato approved these changes Aug 23, 2023
View reviewed changes
@joshuagl
Uncomment note on proto being human readable
04fe7d7 
Signed-off-by: Joshua Lock <joshuagloe@gmail.com>
@joshuagl joshuagl merged commit 9ee63a3 into slsa-framework:main Aug 24, 2023
@joshuagl joshuagl deleted the joshuagl/annotations branch August 24, 2023 09:36
joshuagl added a commit to joshuagl/slsa that referenced this pull request Aug 24, 2023
@joshuagl
impl: require a changelog entry for content changes
d8fc2e4 
As discussed in the conversation for PR slsa-framework#892, specifically:
slsa-framework#892 (comment)
we should treat the What's New page as a Changelog and require
that all content changes to the specification have a corresponding
entry. Update "Contributing to SLSA" to capture this requirement.

Signed-off-by: Joshua Lock <joshuagloe@gmail.com>
@joshuagl joshuagl mentioned this pull request Aug 24, 2023
Merged
joshuagl added a commit that referenced this pull request Aug 24, 2023
@joshuagl
impl: require a changelog entry for content changes (#958)
f7008f0 
As discussed in the conversation for PR #892, specifically:
#892 (comment)
we should treat the "What's New" page as a Changelog and require
that all content changes to the specification have a corresponding
entry. Update "Contributing to SLSA" to capture this requirement.

Signed-off-by: Joshua Lock <joshuagloe@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkLodato MarkLodato MarkLodato approved these changes

@lehors lehors lehors approved these changes

@mlieberman85 mlieberman85 Awaiting requested review from mlieberman85

Assignees
No one assigned
Labels
None yet
Projects
Issue triage
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resolve ambiguity in ResourceDescriptor annotations
3 participants
@joshuagl @MarkLodato @lehors