fixes #22951 - support docker v2 api #7249
Conversation
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
thomasmckay
changed the title
| @repository, [upload_id], :unit_type_id => unit_type_id, | ||
| :unit_keys => unit_keys, | ||
| :generate_metadata => true, :sync_capsule => true) | ||
| rescue |
There was a problem hiding this comment.
Avoid rescuing without specifying an error class.
| :generate_metadata => true, :sync_capsule => true) | ||
| digest = task.output['upload_results'][0]['digest'] | ||
| end | ||
| rescue |
There was a problem hiding this comment.
Avoid rescuing without specifying an error class.
| render json: r | ||
| end | ||
|
|
||
| def push_manifest |
There was a problem hiding this comment.
Assignment Branch Condition size for push_manifest is too high. [78.01/63]
Method has too many lines. [84/30]
theforeman-bot
added
Waiting on contributor
Needs re-review
Needs testing
and removed
Not yet reviewed
Waiting on contributor
labels
thomasmckay
changed the title
| end | ||
|
|
||
| describe "docker push" do | ||
|
|
There was a problem hiding this comment.
Extra empty line detected at block body beginning.
| generate_metadata: true, sync_capsule: true}) | ||
| .returns(stub('task', { | ||
| :output => {'upload_results' => [{ 'digest' => 'sha256:1234' }]} | ||
| })) |
There was a problem hiding this comment.
Indent the right brace the same as the first position after the preceding left parenthesis.
| unit_keys: unit_keys, | ||
| generate_metadata: true, sync_capsule: true}) | ||
| .returns(stub('task', { | ||
| :output => {'upload_results' => [{ 'digest' => 'sha256:1234' }]} |
There was a problem hiding this comment.
Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
| nil, [123], {unit_type_id: 'docker_manifest', | ||
| unit_keys: unit_keys, | ||
| generate_metadata: true, sync_capsule: true}) | ||
| .returns(stub('task', { |
There was a problem hiding this comment.
Redundant curly braces around a hash parameter.
| .with(::Actions::Katello::Repository::ImportUpload, | ||
| nil, [123], {unit_type_id: 'docker_manifest', | ||
| unit_keys: unit_keys, | ||
| generate_metadata: true, sync_capsule: true}) |
There was a problem hiding this comment.
Align the elements of a hash literal if they span more than one line.
| @controller.expects(:sync_task) | ||
| .with(::Actions::Katello::Repository::ImportUpload, | ||
| nil, [123], {unit_type_id: 'docker_manifest', | ||
| unit_keys: unit_keys, |
There was a problem hiding this comment.
Align the elements of a hash literal if they span more than one line.
| # }] | ||
| unit_keys = [{ | ||
| name: "/home/vagrant/code/foreman/tmp/registry_upload/repository_tag.tar", | ||
| size: 10240, |
There was a problem hiding this comment.
Use underscores(_) as decimal mark and separate every 3 digits with them.
| # checksum: Digest::SHA256.hexdigest(content) | ||
| # }] | ||
| unit_keys = [{ | ||
| name: "/home/vagrant/code/foreman/tmp/registry_upload/repository_tag.tar", |
There was a problem hiding this comment.
Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
| result: true, | ||
| count: 1 | ||
| }]) | ||
| unit_keys = ['repository'] |
There was a problem hiding this comment.
Useless assignment to variable - unit_keys.
| name: :create_upload_request, | ||
| result: { 'upload_id' => 123 }, | ||
| count: 2 | ||
| }, { |
There was a problem hiding this comment.
Indent the right brace the same as the first position after the preceding left parenthesis.
| it "push manifest - success" do | ||
| @repository = katello_repositories(:busybox) | ||
| mock_pulp_server([{ | ||
| name: :create_upload_request, |
There was a problem hiding this comment.
Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
thomasmckay
force-pushed
the
registry
branch
2 times, most recently
from
015160a
to
76a2449
Compare
| file.write manifest | ||
| end | ||
| manifest = JSON.parse(manifest) | ||
| rescue |
There was a problem hiding this comment.
Avoid rescuing without specifying an error class.
|
@parthaa @jlsherrill @JacobCallahan - Let the reviews begin! I added a check to see if the registry is config in katello.yml so all the registry routes return 404 if missing. I think there will be enough cases of upload format causing errors that it should be opt-in (aka tech preview) in first pass. Even perhaps in foreman-1.18. Simultaneous uploads are determined by checking for the presence of manifest.json file. I am cleaning up the blob tar files until I can confirm tmp dir will get cleaned out. All the routes require create_personal_access_tokens permissions. Further checks (readable repo, etc.) happen once the user has passed that for the route. I'll continue to write tests. |
thomasmckay
changed the title
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
| render json: r | ||
| end | ||
|
|
||
| #rubocop:disable Metrics/AbcSize |
There was a problem hiding this comment.
Lint/UnneededCopDisableDirective: Unnecessary disabling of Metrics/AbcSize.
thomasmckay
force-pushed
the
registry
branch
2 times, most recently
from
91d6077
to
d5db1f6
Compare
|
@parthaa @jlsherrill @JacobCallahan - This is ready for testing and code review. The docker push functionality will continue to be a find-and-fix effort but it's time to get more users involved in the "find" part of that. There is an upcoming pulp feature that will greatly simplify the push logic https://pulp.plan.io/issues/3497 . If you find issues with push let me know tooling version (either docker or skopeo) as well as the image used so I can repro and work on. Let's not hold up the PR for failed pushes, though. |
|
test fix incoming |
|
@parthaa @jlsherrill - ack? |
|
@thomasmckay few things worked well and few didnt. The following worked great What did not work well for me Were the pull and push expected to work? I am ok acking this if you can create issues for push and pull specifically. One last suggestion. Can you remove the use of katello/app/models/katello/glue/pulp/repo.rb Line 206 in 961148c katello/app/models/katello/glue/pulp/repo.rb Line 785 in 961148c I am guessing if docker pull direct will work, there is no need for the port. Optionally you can add this to the above "pull" issue you might create. Nice work!. |
|
@parthaa - Thanks! |
|
ack |
This feature is not enabled unless the following is in config/settings.plugins.d/katello.yml
To test, create a docker repository. The source url and upstream name can be garbage values since this is just a placeholder repo to push new images to. In my example I made a repo with label "builds" in org with label "examplecorp". Some commands I ran during devel are below.
To test permission scoping, users must have personal access token permissions (view_personal_access_tokens, create_personal_access_tokens, revoke_personal_access_tokens). Limiting the user to a specific lifecycle environment will also reduce their scope of search and pulling images.