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

RemoteArtifacts checksum type check #1209

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

pavelpicka
Copy link
Collaborator

RemoteArtifacts are checked for forbidden checksums.

closes: #8423
https://pulp.plan.io/issues/8423

@pulpbot
Copy link
Member

pulpbot commented Mar 24, 2021

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

@daviddavis
Copy link
Contributor

The logic here is not quite correct. We want to allow a RemoteArtifact if it (1) has no checksums or (2) has an allowed checksum. So it can have forbidden checksum(s) if also has an allowed checksum. Something like this:

def validate_checksums(self):
    """Check that a RemoteArtifact either doesn't have checksums or has no allowed checksums.""""
    if len([c for c in ALL_KNOWN_CHECKSUMS if get_attr(self, c, False)]) == 0:
        # doesn't have any checksums
        return
    if len([c for c in Artifact.DIGEST_FIELDS if get_attr(self, c, False)]) == 0:
        # doesn't have any allowed checksums
        raise UnsupportedDigestValidationError("...")

@pavelpicka pavelpicka force-pushed the 8423-ra-checksums-check branch 2 times, most recently from 267f07b to 900f281 Compare March 25, 2021 14:07
@daviddavis
Copy link
Contributor

Tests look good. Can you add a test that checks a RemoteArtifact with an allowed AND a forbidden checksum? It should not raise an exception.

]
):
raise UnsupportedDigestValidationError(
"RemoteArtifact contains forbidden checksum type, thus cannot be synced."
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if users know what RemoteArtifacts are. I would maybe say "On-demand content".

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also include self.url in the message to help users track down the problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also changed in CHANGES message

@pavelpicka pavelpicka force-pushed the 8423-ra-checksums-check branch 2 times, most recently from 2548098 to 93b5d11 Compare March 26, 2021 11:02
):
raise UnsupportedDigestValidationError(
_(
"On-demand content {} contains forbidden checksum type, thus cannot be synced."
Copy link
Member

Choose a reason for hiding this comment

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

if i may suggest : On-demand content located at the url {}...

[
checksum_type
for checksum_type in ALL_KNOWN_CONTENT_CHECKSUMS
if getattr(self, checksum_type, False)
Copy link
Member

Choose a reason for hiding this comment

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

in theory the AttributeError should never be raised

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in theory yes, but I'd rather stay explicit

@@ -0,0 +1 @@
On-demand content are checked for not allowed checksum types and Pulp raises an exception if any found.
Copy link
Member

Choose a reason for hiding this comment

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

s/are checked/is checked/

Copy link
Member

Choose a reason for hiding this comment

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

On-demand content is checked for forbidden checksum types and Pulp raises an exception when those are exclusively found.

Copy link
Member

Choose a reason for hiding this comment

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

@daviddavis exclusively or solely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both work. I might even use something simpler: "Added sync check that raises error when only forbidden checksums are found for on-demand content".

RemoteArtifacts are checked for forbidden checksums.

closes: #8423
https://pulp.plan.io/issues/8423
@daviddavis daviddavis merged commit ee0684a into pulp:master Mar 26, 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

4 participants