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

records: update json schema to include container images #3391

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

nancyhamdan
Copy link
Member

fixes #3386
updated records' JSON schema to include container images metadata fields

@nancyhamdan nancyhamdan requested review from ArtemisLav and removed request for ArtemisLav July 11, 2023 10:00
@nancyhamdan nancyhamdan changed the title records: update json schema to include container images records: update json schema to include container images (WIP) Aug 8, 2023
"items": {
"properties": {
"description": {
"description": "A brief description of the container image",
Copy link
Member

Choose a reason for hiding this comment

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

It may be useful to specify that the image is used for analysing the data, for example:

  • Description of the container image that is recommended for analysing these data

(Alternatively, if we could have several images linked here, one recommended for analysis, one how the data was born, we could even introduce a new property called "purpose" or some such... But at this moment it's probably an overkill, since we plan to store only the images useful for data analyses. Unless there could be several images in the future for different purposes... CC @katilp )

Copy link
Member

Choose a reason for hiding this comment

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

Yes, for now, we do not have plans to link to other images than that to be used for analysing the data.

(I could only imagine a separate image for MC to generate a new similar MC dataset with some changes in the input but we are not providing those images and I'd rather not link them to the records in any case)

"type": "string"
},
"recid": {
"description": "The internal ID of the container image, if it is another Open Data record",
Copy link
Member

Choose a reason for hiding this comment

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

Let's start again by the same word as the field is called (recid) so that readers can form better mental association:

  • Record ID of the container image (if it exists as another Open Data record)

"type": "string"
},
"registry": {
"description": "The type of registry the image can be found at",
Copy link
Member

Choose a reason for hiding this comment

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

It may be useful to provide example values in the documentation:

  • Registry type where the image can be found (e.g. dockerhub, cerngitlab)

"type": "string"
},
"uri": {
"description": "The URI for finding the container image",
Copy link
Member

Choose a reason for hiding this comment

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

Again here:

  • URI where the container image is located

However, we may perhaps be better off using a common term FQIN here to denote fully-qualified image name concept, i.e. the field would be called "fqin" instead of "uri" and the description would explain it as follows:

  • Fully Qualified Image Name (FQIN) where the container image is located (repository/name:tag)

We can discuss live also with @katilp before merging

Copy link
Member

Choose a reason for hiding this comment

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

P.S. BTW it the "fqin" field name risks looking a bit weird, we could use some longer name variant such as "fully_qualified_name", or just "name", and document that the name should be FQIN in the schema.

Copy link
Member

Choose a reason for hiding this comment

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

Good points! I would go for "name" and documenting it in the schema as @tiborsimko proposed

@nancyhamdan nancyhamdan force-pushed the cms-nancy-schema-include-record-image branch from b136490 to e01bdfe Compare August 14, 2023 09:46
@nancyhamdan nancyhamdan changed the title records: update json schema to include container images (WIP) records: update json schema to include container images Aug 14, 2023
@tiborsimko tiborsimko force-pushed the cms-nancy-schema-include-record-image branch from e01bdfe to c3707a6 Compare August 21, 2023 15:11
Copy link
Member

@tiborsimko tiborsimko left a comment

Choose a reason for hiding this comment

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

Thanks, rebased against latest QA.

After merge, please prepare another PR with a small example file containing the data as well as the output format Jinja snippet displaying it, as we had talked about IRL.

@tiborsimko tiborsimko merged commit c3707a6 into qa Aug 21, 2023
14 checks passed
@tiborsimko tiborsimko deleted the cms-nancy-schema-include-record-image branch August 21, 2023 15:30
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.

3 participants