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

feat: adding sonatype central support #3130

Merged
merged 10 commits into from
May 26, 2024

Conversation

Andrapyre
Copy link
Contributor

@Andrapyre Andrapyre commented Apr 24, 2024

Description

This PR adds support for the new Sonatype Central publishing api, documented here and here. To use, ensure that your environment variables contain the appropriate credentials and configure your module like so in build.sc:

import $ivy.`com.lihaoyi::mill-contrib-sonatypecentral:`
import mill.contrib.sonatypecentral.SonatypeCentralPublishModule

object mymodule extends SonatypeCentralPublishModule {
  ...
}

New Dependencies

This PR introduces the following new dependency:

ivy"com.lumidion::sonatype-central-client-requests:0.2.0" 

@Andrapyre
Copy link
Contributor Author

@lihaoyi , could you take a look at this? A few open questions:

  1. What's the best way to make these changes accessible to the user? By altering the current PublishModule or by creating a new SonatypeCentralPublishModule or something like it?
  2. There are a number of changes to the PublishModule affecting binary compat. Basically, I refactored as many publish options as possible into two case classes (one for legacy publishing and one for central publishing) that the user can configure, instead of having the user pass them directly into the publish function. Is breaking binary compat acceptable here, since mill still hasn't hit version 1.0.0?
  3. The current implementation outputs publishable jar files to ./out/central-publish. Let me know if that should change (I don't think the older Sonatype implementation outputted any jar files, unless I'm mistaken) or if the directory should be configurable.

build.sc Outdated Show resolved Hide resolved
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution. We have a lot of other publishers, that currently reside as contrib plugins. Since your PR a) is rather new, b) touches a lot of heavily used and depend-on functionality which is c) rather proven and tested to work, I'd suggest to start this addition as a contrib module. So users can start to adopt this new API without compromizing existing setups.

That is also the best way to provide an alternative PublishModule v2, starting with the refactorings you mentioned applied to it. I haven't reviewed this PR in detail, but since you mentioned that you introduced cases classes, which themselves aren't ideal to maintain binary compatibility, I suggest to go with a contrib module for now.

If that's not what you want, I'd like to have some smaller change set, preserved binary compatibility, some docs describing how the new mechanism works and how it differs to the existing logic and some test coverage of the newly introduced functionality, if possible.

Please also keep in mind, that the current publish logic in PublishModule is also used for remote repositorries other than Sonatype OSS / Maven Central, but for many local / company wide repositories like Nexus.

@Andrapyre
Copy link
Contributor Author

I've updated with requested changes and documentation. Before I go on to add tests, could you review and approve the current implementation?

@Andrapyre
Copy link
Contributor Author

@lefou and @nightscape , just making sure this doesn't get lost.

@nightscape
Copy link
Contributor

@lefou how would the separate PublishModule work with e.g. mill-ci-release which extends PublishModule and provides additional functionality on top?

@nightscape
Copy link
Contributor

@lefou ping 😉

@Andrapyre
Copy link
Contributor Author

@nightscape , mill-ci-release would need a new module that extends SonatypeCentralPublishModule. The two publishing processes (standard and Sonatype Central) are different enough that @lefou is probably right that these should live in separate modules.

@lefou
Copy link
Member

lefou commented May 16, 2024

I can't say much about mill-ci-release, which I don't use. (I think publishing with Mill built-in support is already straight forward, given that you have a properly setup pgp/gpg key.) If mill-ci-release needs to be aware of any additional target not already provided by PublishModule (which you could mix-in), then it most likely also needs extra customizations.

@lefou lefou marked this pull request as draft May 16, 2024 13:23
lefou
lefou previously requested changes May 16, 2024
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

As written before:

Thank you for this contribution. We have a lot of other publishers, that currently reside as contrib plugins. Since your PR a) is rather new, b) touches a lot of heavily used and depend-on functionality which is c) rather proven and tested to work, I'd suggest to start this addition as a contrib module. So users can start to adopt this new API without compromizing existing setups.

That is also the best way to provide an alternative PublishModule v2, starting with the refactorings you mentioned applied to it. I haven't reviewed this PR in detail, but since you mentioned that you introduced cases classes, which themselves aren't ideal to maintain binary compatibility, I suggest to go with a contrib module for now.

If that's not what you want, I'd like to have some smaller change set, preserved binary compatibility, some docs describing how the new mechanism works and how it differs to the existing logic and some test coverage of the newly introduced functionality, if possible.

Please also keep in mind, that the current publish logic in PublishModule is also used for remote repositorries other than Sonatype OSS / Maven Central, but for many local / company wide repositories like Nexus.

@Andrapyre
Copy link
Contributor Author

@lefou , this PR is ready to be reviewed. I have made all requested changes (e.g. this is now a contrib module, no changes to bincompat, removed case classes, etc). Could you explain why this PR was changed to draft with a reference to your previous comment?

@lefou
Copy link
Member

lefou commented May 17, 2024

@Andrapyre I somehow missed, that you already changed to PR. Sorry for that.

@lefou lefou marked this pull request as ready for review May 17, 2024 06:43
@lefou lefou dismissed their stale review May 17, 2024 06:49

by accident

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Can you please update the PR description to reflect the current state?

@Andrapyre
Copy link
Contributor Author

Can you please update the PR description to reflect the current state?

Done.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

We're almost there. One nit, otherwise this looks good to me.

scalalib/src/mill/scalalib/PublishModule.scala Outdated Show resolved Hide resolved
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

@lefou lefou merged commit 55eb046 into com-lihaoyi:main May 26, 2024
39 checks passed
@lefou lefou added this to the 0.11.8 milestone May 26, 2024
lefou pushed a commit that referenced this pull request May 27, 2024
Class method name changes that were made in the process of approving
#3130 left out corresponding
documentation changes. This PR adjusts the documentation so that it
reflects the final implementation.

Pull request: #3187
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.

3 participants