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

Try and simplify publishing setup #3523

Merged
merged 11 commits into from
Sep 12, 2024
Merged

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Sep 12, 2024

This PR attempts to in-source most of the convenience provided by https://github.com/ckipp01/mill-ci-release into Mill proper. The boilerplate necessary to call publishAll on mac laptops or linux github actions environments was trivial and straightforward, so we just try to automate it:

  1. Prefix all environment variables with MILL_: MILL_SONATYPE_USERNAME, MILL_SONATYPE_PASSWORD, MILL_PGP_SECRET_BASE64, MILL_PGP_PASSPHRASE to try and add some rudimentary ownership and namespacing

  2. Auto-detect and import MILL_PGP_SECRET_BASE64, and auto-detect MILL_PGP_PASSPHRASE and include it in the default GPG arguments. MILL_SONATYPE_USERNAME and MILL_SONATYPE_PASSWORD were already auto-detected

  3. Flesh out the defaultGpgArgs to allow the optional passphrase to be passed in, and include all the flags that Mill uses in its own CI publishing job

  4. Set the defaults for readTimeout, awaitTimeout, stagingRelease, publishArtifacts to match the common convention

  5. Set release = true by default

Updated the ci/release-maven.sh script to publish using a bootstrap version of Mill, so once this is merged I can immediately test it without needing to go through a rebootstrapping process. The current behavior when passing in flags explicitly should be unchanged, this just acts a lot more aggressively at setting defaults

@lihaoyi lihaoyi changed the title Test Fork Github Actions Try and simplify publishing setup Sep 12, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 12, 2024

@ckipp01 as the author of mill-ci-release would love to have your review on this if possible

Copy link
Collaborator

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Overall I'm really happy to see this being inlined! I think small Quality of Life improvements like this go a long way! Here is a couple things that I'm not 100% sure this PR covers but I'd recommend making sure they are either covered already or should be considered. I found a lot of newcomers get bit by this and Sonaytpe just makes it confusing:

  • Make sure there is an easy way to set the sontaype host uri. I've found having small helper like this quite useful..
  • Warn if necessary things for Sonatype aren't configured to avoid confusion. Sontaype will just fail without clear messaging as to why. So I've found including checks for things like license and developers which are necessary really helpful. Here is how I did it. This would be nice to include.

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.

I never used mill-ci-release but I think having better and sensible defaults is nice.

If this is a directly code in-sourcing, please address the copyright, since mill-ci-release has a compatible but different license.

@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 12, 2024

Thanks @ckipp01! Will take that into account. @lefou this isn't a literal in-sourcing, most of the config defaults are taken from Mill's own build

@lefou
Copy link
Member

lefou commented Sep 12, 2024

Checking validation rules should not be hardcoded into publish IMHO. Each remote reporsitory can implement different rules, local publishing typically has non, so the best would be to have some new convenient check* target with some preset for Sonatype OSS Repositoy Hosting. We could create a predefined map from repo URL to Ruleset to configure which checks we should run.

@lihaoyi lihaoyi merged commit 4d18511 into com-lihaoyi:main Sep 12, 2024
23 checks passed
@lihaoyi lihaoyi added this to the 0.12.0 milestone Sep 12, 2024
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