Add docker content models. #260
Conversation
|
Hello @jortel! Thanks for updating the PR.
Comment last updated on September 10, 2018 at 20:38 Hours UTC |
pulp_docker/app/models.py
Outdated
| This content has no artifacts. | ||
|
|
||
| Fields: | ||
| digest (models.CharField): The manifest digest. |
There was a problem hiding this comment.
Maybe more helpful: The SHA256 (is that what this is?) if the manifest file
There was a problem hiding this comment.
as of now the sha256 is used, i would like to leave this opened in case some other algorithms will come in place later.
There was a problem hiding this comment.
Cool, how about "The digest of the manifest file."
pulp_docker/app/models.py
Outdated
| """ | ||
| A blob defined within a manifest. | ||
|
|
||
| This content has no artifacts. |
There was a problem hiding this comment.
A ManifestList actually does have a file on disk. I think maybe this docbloc is an accidental copy/paste?
| media_type = models.CharField( | ||
| max_length=60, | ||
| choices=( | ||
| (MEDIA_TYPE.MANIFEST_LIST, MEDIA_TYPE.MANIFEST_LIST), |
There was a problem hiding this comment.
These 2 choices are the same. I think there is only one choice, maybe this one doesn't need to be a choice field.
There was a problem hiding this comment.
that's true that there is just 1 choice - for now ( you never know what will come next)
- the value of this field will be validated at the db level.
There was a problem hiding this comment.
Just noticed that this is (visible choice, internal choice), so this is just 1 choice. It seems reasonable to expect docker to add more choices in the future so this is fine.
pulp_docker/app/models.py
Outdated
| manifest_list = models.ForeignKey( | ||
| ManifestList, null=True, related_name='tags', on_delete=models.CASCADE) | ||
|
|
||
| class Meta: |
There was a problem hiding this comment.
Nonblocking question: Is there a way to enforce that a specific tag cannot reference both a manifest and a manifest list?
There was a problem hiding this comment.
@dkliban, do you know of a way that is compatible with bulk_create()?
| (MEDIA_TYPE.CONFIG_BLOB, MEDIA_TYPE.CONFIG_BLOB), | ||
| (MEDIA_TYPE.REGULAR_BLOB, MEDIA_TYPE.REGULAR_BLOB), | ||
| (MEDIA_TYPE.FOREIGN_BLOB, MEDIA_TYPE.FOREIGN_BLOB), | ||
| )) |
There was a problem hiding this comment.
Should this also reference an artifact? If so, it seems reasonable to leave out for now and add when there are actual units with artifacts.
There was a problem hiding this comment.
The artifact relationship is already supported generically by ContentArtifact.
pulp_docker/app/models.py
Outdated
|
|
||
| class ManifestList(Content): | ||
| """ | ||
| A blob defined within a manifest. |
| TYPE = 'manifest-blob' | ||
|
|
||
| digest = models.CharField(max_length=255) | ||
| media_type = models.CharField( |
There was a problem hiding this comment.
@asmacdo when you will be implementing sync, at the parsing of the manifest.json: if the information for the mediatype of the blob will be available, then use that info, otherwise set the value to MEDIA_TYPE.REGULAR_BLOB
** for the background, in schema1 there is no information available for blobs, just its digest. But also schema 1 can cope only with application/vnd.docker.image.rootfs.diff.tar.gzip mediatype
pulp_docker/app/models.py
Outdated
| """ | ||
| A docker manifest. | ||
|
|
||
| This content has no artifacts. |
There was a problem hiding this comment.
This content should have artifact --> manifest.json
manifest.json is the file we fetch and parse, and based on what we parsed we put this info onto the model and save, including the json file.
Same for ManifestList.
|
@jortel, before you merge, the CI failure looks important: Think we ought to have all of the plugins namespace their tables? |
|
@asmacdo, The tables are already namespaced by django. The docker plugin tables are namespaced under the 'app' name. For example: the |
closes #3401
https://pulp.plan.io/issues/3401
I'm not convinced we want to use
choice=for media_type fields but did at the direction of the story.