-
Notifications
You must be signed in to change notification settings - Fork 677
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
Add artifact schema and tests #942
Conversation
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.
Approving even with the sugested change in case we want to fix that in a separate PR.
@sajayantony #944 is merged. This should be ready to rebase and clean up the ioutil references. Thanks! |
I need to resubmit this with the field name changes now :) |
2dd253f
to
2bf82b9
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.
LGTM with a couple minor changes. I think it would be helpful to include some valid minimal artifacts here too. The three examples I'm thinking of are
- mediaType only (kinda useless but valid)
- mediaType + artifactType + blobs
- mediaType + artifactType + annotations
I'm happy to do that as a separate PR if that's easier.
schema/artifact-manifest-schema.json
Outdated
@@ -0,0 +1,29 @@ | |||
{ | |||
"description": "OpenContainer Artifact Manifest Specification", | |||
"$schema": "https://json-schema.org/draft-04/schema#", |
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.
From the spec, this needs to be the exact string and we weren't supposed to have changed it to https: https://json-schema.org/understanding-json-schema/reference/schema.html
"$schema": "https://json-schema.org/draft-04/schema#", | |
"$schema": "http://json-schema.org/draft-04/schema#", |
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've opened - #961 to fix up the remaining draft 4 identifier strings.
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.
Thanks for pointing me to #931 @sudo-bmitch. I’ve closed #961
schema/artifact_test.go
Outdated
"subject": { | ||
"mediaType": "application/vnd.oci.image.manifest.v1+json", | ||
"digest": "sha256:c86f7763873b6c0aae22d963bab59b4f5debbed6685761b5951584f6efb0633b" | ||
}, | ||
"annotations": { | ||
"key1": "value1", | ||
"key2": "value2" | ||
} |
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, indentation needs an extra space
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 went ahead and reformatted all the examples. It seems that certain properties had spaces after the property names and some didn't and I think those that have spaces were incorrectly formatted.
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.
Please feel to add more examples. I'm hoping to get the baseline framework and schema in first.
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.
@sudo-bmitch hope I've addressed the issues.
Need to validate that that the tests catch example issues like |
bbb501a
to
2b2e760
Compare
Signed-off-by: Sajay Antony <[email protected]>
2b2e760
to
1760050
Compare
@shizhMSFT thanks for fixing #960 - artifact examples were not validated in this PR and your PR helped :) |
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.
LGTM
Bumping this issue to get feedback from other maintainers @opencontainers/image-spec-maintainers |
Closing this one because of #999. |
Signed-off-by: Sajay Antony [email protected]