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

Artifact checksums report #1173

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Conversation

pavelpicka
Copy link
Collaborator

pulpcore-manager handle-artifact-checksums --report reports local and remote artifacts with forbidden checksum type and approx size of it.

re #7986
https://pulp.plan.io/issues/7986

@pulpbot
Copy link
Member

pulpbot commented Mar 5, 2021

Attached issue: https://pulp.plan.io/issues/7986

@daviddavis
Copy link
Contributor

Looking good. Will test it out later today.

@pavelpicka pavelpicka force-pushed the 7986-checksum-report branch 2 times, most recently from ee17438 to 3f766ac Compare March 5, 2021 15:29
@daviddavis
Copy link
Contributor

It looks like this is missing the has_content() change from the PoC:

AttributeError: type object 'RepositoryVersion' has no attribute 'has_content'

Bonus points if you write some unit tests for this method.

@daviddavis
Copy link
Contributor

Overall, this looks great to me. 👍 We may have to tweak the criteria around reporting on-demand content but I think with the tech preview label, we can merge this once the issues I pointed out are fixed.

@bmbouter @ipanova maybe you too can review as well?

@ipanova
Copy link
Member

ipanova commented Mar 5, 2021

Overall, this looks great to me. +1 We may have to tweak the criteria around reporting on-demand content but I think with the tech preview label, we can merge this once the issues I pointed out are fixed.

@bmbouter @ipanova maybe you too can review as well?

yes, i can get to this on monday.

@ipanova
Copy link
Member

ipanova commented Mar 10, 2021

I did some testing and it seems to report correct thing.

@pavelpicka pavelpicka force-pushed the 7986-checksum-report branch 2 times, most recently from cb51f9d to a3aff8d Compare March 15, 2021 12:19

Returns None
"""
for repo_version in repo_versions.iterator():
Copy link
Member

Choose a reason for hiding this comment

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

this produces wrong url. you need to call cast() on the repo and also add the versions part to the url

Affected repository versions with present content:
/repositories/core/repository/58c1c1a0-4b6a-4140-a959-4bbbe81245aa/1/

Copy link
Contributor

@dralley dralley Mar 15, 2021

Choose a reason for hiding this comment

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

My bad, I think the correct thing to use would have been repository.pulp_type rather than repository.get_pulp_type().

I think that should make casting unnecessary. +1 to versions at the end.

Copy link
Member

Choose a reason for hiding this comment

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

@dralley i can confirm repository.pulp_type works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used .pulp_type instead casting.

@pavelpicka pavelpicka force-pushed the 7986-checksum-report branch 2 times, most recently from 86e0ac9 to 9880caf Compare March 15, 2021 15:44
reports local and remote artifacts with forbidden checksum type.

re #7986
https://pulp.plan.io/issues/7986
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.

Thanks!

@ipanova ipanova merged commit e7af852 into pulp:master Mar 15, 2021
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.

None yet

5 participants