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 distributionManagement by default #5856

Closed
wants to merge 1 commit into from

Conversation

ppkarwasz
Copy link

Usually distributionManagement is not the right place to download artifacts, the URLs are often write-only. Therefore usage of these repositories should be disabled by default.

Closes #5854.

Usually `distributionManagement` is not the right place to download
artifacts, the URLs are often **write-only**. Therefore usage of these
repositories should be disabled by default.

Closes bndtools#5854.

Signed-off-by: Piotr P. Karwasz <[email protected]>
@pkriens
Copy link
Member

pkriens commented Oct 31, 2023

@rotty3000 could you take a quick look to review? It is a one line change in the code, I can look at the comments and this was your expertise?

@rotty3000
Copy link
Contributor

Hey guys. Thanks @ppkarwasz for the contribution! :)

I can't speak directly for @timothyjward who implemented this, however, if I had to guess his intentions, the idea was (probably) that finding a previously released artifact of the current project to baseline against is almost certainly going to be where you are planning to distribute the current artifact on release. I'd find it highly unlikely that it would exist in any of the repositories you have configured for dependencies, at least that would seem awkward to me.

However, if your distributionManagement repo IS write only, then maybe just set it to false? I guess my point is that I'd like to see more than anecdotal evidence about the common case being that the distributionManagement repo is write only before making a change to a boolean which has been this way since pretty much day 1 of the plugin. And there's a risk of breaking things here.

@ppkarwasz
Copy link
Author

@rotty3000,

In practice most open source artifacts end up on Maven Central (which is configured by default in <repositories>), while publication to Maven Central goes through write-only staging repos (like OSSRH). According to the documentation the distributionManagement/repository element is used to only upload elements.

I confirmed it with @cstamas on the Maven Slack (hence a proof by obscure reference ;-) ).

Of course, we set includeDistributionManagement to false in our projects, so this is more of a wish list than a bug.

@pkriens
Copy link
Member

pkriens commented Nov 1, 2023

would it be an idea to make the boolean depend on the write only status of distributionManagement ?

@timothyjward could you also shine your light on this?

@rotty3000
Copy link
Contributor

rotty3000 commented Nov 1, 2023

would it be an idea to make the boolean depend on the write only status of distributionManagement ?

If that was something you could easily check then it would be ideal.

However, I fear that might be a complex network check to make for not much gain, no?

I'm just afraid that changing the default might break existing setups without any significant advantage (people tend to update plugin versions and justifiably assume their configurations are backward compatible). So even if we accept the "proof by obscure reference", which I'm fine with (respect to @cstamas) we shouldn't break people, even if it's annoying.

But I'm just one voice in a choir. Consensus rules.

@pkriens
Copy link
Member

pkriens commented Nov 1, 2023

@ppkarwasz I am sorry I asked you to make a PR but you mind if we close the issue and this? I am worried about the backward compatibility and unless we can figure out the r/w status of the distribution management, I agree with @rotty3000 we will annoy people

@ppkarwasz
Copy link
Author

ppkarwasz commented Nov 1, 2023

@pkriens,

No problem. As I already stated, it is more of a wishlist, since I think this should be the right setting for most Maven projects. But I understand the woes of BC, this is not worth breaking people's builds.

@ppkarwasz ppkarwasz closed this Nov 1, 2023
@timothyjward
Copy link
Contributor

@timothyjward could you also shine your light on this?

I’ll try!

while publication to Maven Central goes through write-only staging repos (like OSSRH

I dispute this - the repositories are not, In any case I have ever seen, write only. The Apache release process relies on the fact that people can access the staged artifacts to verify them before they get released to central.

There actually was a use case that caused the use of distribution management, specifically related to release verification. If I stage a release (either through CI or locally) and I want someone else to be able to mvn verify the SCM tag then the baseline jar (i.e. the staged binary) is potentially only in the distribution management repo and nowhere else. To correctly validate versioning the baseline plugin therefore needs to look there.

As an aside I believe that the distribution management check also honours the maven defined properties for overriding the distribution repository location. This makes it easy to do validation with “one time” staging repos as used in Apache.

@ppkarwasz
Copy link
Author

There actually was a use case that caused the use of distribution management, specifically related to release verification. If I stage a release (either through CI or locally) and I want someone else to be able to mvn verify the SCM tag then the baseline jar (i.e. the staged binary) is potentially only in the distribution management repo and nowhere else. To correctly validate versioning the baseline plugin therefore needs to look there.

Good point, I haven't thought about that.

Can you clarify one thing: why would baseline download the current release from the repo, shouldn't it compare the local artifacts with the previous release?

@ppkarwasz ppkarwasz deleted the distribution-management branch November 1, 2023 18:36
@cstamas
Copy link

cstamas commented Nov 1, 2023

Oh true: on ASF release, during release of version X this plugin tries to download version X (and fails), same version that is in this very current moment (invocation) being released...

@timothyjward
Copy link
Contributor

Can you clarify one thing: why would baseline download the current release from the repo, shouldn't it compare the local artifacts with the previous release?

The difference in behaviour is supposed to be triggered based on the version of the maven module being built.

If the module is a SNAPSHOT then we look for the highest release we can find less than the version of the snapshot we’re building.

If the module is a release version then we first try to find the release - if it exists then we baseline by looking for differences, as there shouldn’t be any. If the release can’t be found then we fall back to the “highest version less than” behaviour.

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.

Don't use distribution management in Baseline Maven plugin by default
5 participants