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

Disable md5 publishing #164

Closed
wants to merge 2 commits into from

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Nov 2, 2022

Currently all registered checksums are computed and published, but in some cases it might be usefully to be able to verify a checksum but we don't want to publish it.

This also includes disabling the md5 checksum, we publish sha-256 > 4 years and warn about md5 > 1 year now so it seems valid to stop publishing it now at all.

@merks
Copy link
Contributor

merks commented Nov 2, 2022

Should we perhaps also stop publishing (not start publishing) sha-1)? I.e., perhaps sha-1 can/should be used to populate only when getting it from Maven but not when using p2 to publish a new repository given that both sha-256 and sha-512 are now published and only one of the latter two will ever be used at runtime such that a sha-1will never be consumed so is just useless overhead...

@laeubi
Copy link
Member Author

laeubi commented Nov 2, 2022

Yes if we eventually add sha-1 (the PR is still pending) it could be disabled like md5 fro publishing.

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

It looks fine, but I'm not sure if the default value is returned from the getAttribute call...

@laeubi laeubi force-pushed the disable_md5_publishing branch 4 times, most recently from c64d3ac to d3d890d Compare November 2, 2022 14:59
@laeubi
Copy link
Member Author

laeubi commented Nov 2, 2022

Lets first get the option to disable a checksum in:

@laeubi
Copy link
Member Author

laeubi commented Nov 3, 2022

@mickaelistria please update your review, @akurtakov thats how far I was able to get with the tests, I'll keep this PR just in case someone likes to build on top of it.

@akurtakov
Copy link
Member

Just so I get things correct, what prevents this patch from getting merged is failing tests?

@laeubi
Copy link
Member Author

laeubi commented Nov 4, 2022

Just so I get things correct, what prevents this patch from getting merged is failing tests?

Yes tests need to be adjust (or deleted or ...) because they depend on md5 calculated (what is no longer the case)

@akurtakov
Copy link
Member

Would you please reset this PR on master? aka removing my merge commit.

@laeubi
Copy link
Member Author

laeubi commented Nov 8, 2022

Would you please reset this PR on master? aka removing my merge commit.

Done!

This is disabling the md5 checksum, we publish sha-256 > 4
years and warn about md5 > 1 year now so it seems valid to stop
publishing it now at all.
@akurtakov
Copy link
Member

Pushed as e60c5f1

@akurtakov akurtakov closed this Dec 5, 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.

4 participants