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

Add support for pushing manifest lists to the registry #597

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

lubosmj
Copy link
Member

@lubosmj lubosmj commented Feb 21, 2022

closes #469

@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch 5 times, most recently from f931df5 to ade04c8 Compare February 21, 2022 08:55
@lubosmj lubosmj marked this pull request as ready for review February 21, 2022 13:03
@lubosmj lubosmj marked this pull request as draft February 23, 2022 12:43
@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch 2 times, most recently from 9ebed5f to 79aca70 Compare February 27, 2022 18:00
@lubosmj lubosmj marked this pull request as ready for review February 27, 2022 18:22
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
@lubosmj lubosmj marked this pull request as draft March 2, 2022 17:49
@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch 9 times, most recently from bfe6c23 to ac4dd01 Compare March 7, 2022 16:45
@lubosmj lubosmj marked this pull request as ready for review March 7, 2022 16:47
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
pulp_container/app/registry_api.py Outdated Show resolved Hide resolved
if has_task_completed(dispatched_task):
return ManifestResponse(manifest, path, request, status=201)

def _save_manifest(self, artifact, manifest_digest, request, config_blob=None):
Copy link
Member

Choose a reason for hiding this comment

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

nice :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd place request in front of artifact

Copy link
Member

Choose a reason for hiding this comment

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

actually you're using only content_type, no need to pass whole request object - request.content_type would do.

blobs.add(digest)
raise BlobInvalid(digest=config_digest)

try:
Copy link
Member

Choose a reason for hiding this comment

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

why this is needed? you can just use the repository object directly from line 843

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, the try-except block is redundant and it can be simply rewritten as version = distribution.repository_version or repository.latest_version().

I wanted to get a repository version from the distribution since the distribution may point to a non-latest repository version.

Copy link
Member

@ipanova ipanova Mar 9, 2022

Choose a reason for hiding this comment

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

can you just use repository object from line 843 and do repository.latest_version()?

@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch from ac4dd01 to e9567d0 Compare March 9, 2022 19:30
@lubosmj lubosmj marked this pull request as draft March 9, 2022 19:30
@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch 2 times, most recently from e774eb7 to ee22aa1 Compare March 10, 2022 08:12
@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch from ee22aa1 to 76b45be Compare March 10, 2022 08:14
@lubosmj lubosmj marked this pull request as ready for review March 10, 2022 08:34
@lubosmj lubosmj requested a review from ipanova March 10, 2022 08:34
Copy link
Member

@ipanova ipanova left a comment

Choose a reason for hiding this comment

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

please update this line https://github.com/pulp/pulp_container/pull/597/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR910 to:
version = repository.latest_version()
since you are pushing to content, it will always be into a push-repo-type where distribution will always point to a repository and not repo version.

@lubosmj lubosmj force-pushed the push-manifest-lists-api-469 branch from 76b45be to 1843a04 Compare March 10, 2022 15:07
@lubosmj
Copy link
Member Author

lubosmj commented Mar 10, 2022

please update this line https://github.com/pulp/pulp_container/pull/597/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR910 to: version = repository.latest_version() since you are pushing to content, it will always be into a push-repo-type where distribution will always point to a repository and not repo version.

Done.

@ipanova ipanova merged commit 5548b9a into pulp:main Mar 10, 2022
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.

As a user i want to push a manifest list using container registry API
2 participants