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
Fixes #27524 - fix docker cv publishing #8259
Fixes #27524 - fix docker cv publishing #8259
Conversation
Issues: #27524 |
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 |
@ianballou, this pull request is currently not mergeable. Please rebase against the master branch and push again. If you have a remote called 'upstream' that points to this repository, you can do this by running:
This message was auto-generated by Foreman's prprocessor |
b5ffedf
to
c3d6bd0
Compare
a1e9dd0
to
ad8ae42
Compare
gpg_key_id: <%= ActiveRecord::FixtureSet.identify(:fedora_gpg_key) %> | ||
unprotected: <%= true %> | ||
url: 'https://registry-1.docker.io' | ||
docker_upstream_name: 'fedora/ssh' |
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.
lets not add another docker repo to fixtures. Lets try to adapt one that already exists if what we have isn't sufficient.
ensure | ||
delete_repo(@ssh) | ||
end | ||
|
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'm not sure this test actually reproduces the problem? I think you'd have to then create some content_view repo as well, and then index, passing in the source repo. Although there'd be nothing inthe content view repo. Maybe you could copy units over to the cv repo prior
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.
Ok, I'll bolster this a bit
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 updated the test.
d9a6175
to
d69bb83
Compare
Looks like I need to re record some VCRs before getting all the tests to pass. |
Works :) Before patch: 2019-08-08T13:56:44 [E|bac|] PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "index_katello_docker_tags_on_pulp_id"
13:56:44 rails.1 | | DETAIL: Key (pulp_id)=(5b3b0767-4e0b-4029-b7c7-da1a618aade7) already exists.
13:56:44 rails.1 | | : insert into katello_docker_tags (repository_id, pulp_id, name,docker_taggable_id,docker_taggable_type)
13:56:44 rails.1 | | select 12 as repository_id, pulp_id, name,docker_taggable_id,docker_taggable_type from katello_docker_tags
13:56:44 rails.1 | | where repository_id = 11 and pulp_id not in (select pulp_id
13:56:44 rails.1 | | from katello_docker_tags where repository_id = 12) (ActiveRecord::RecordNotUnique) After patch: |
7ef92c7
to
9140704
Compare
rebased and now I'm getting some Pulp3 Ansible errors, hm... |
[test katello] |
assert_equal @repo.docker_tags.count, 248 | ||
service.copy_contents(@repo_copy) | ||
@repo_copy.index_content(:source_repository => @repo) | ||
assert_equal @repo_copy.docker_tags.count, 248 |
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.
we can't rely on a specific count, as the next person that runs the test in live mode may get a different result. Something like
assert @repo.docker_tags.count > 0
assert_equal @repo.docker_tags.count, @repo_copy.docker_tags.count
should be sufficient. Also i think this test needs to be re-recorded after rebasing against master to get my vcr fixes that avoid duplicate recordings
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.
Rebased and test fixed
9140704
to
508a16a
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.
Thanks @ianballou !!
To reproduce issue:
To test this patch: