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

Adopt gha-scala-library-release process #32

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

Divs-B
Copy link
Contributor

@Divs-B Divs-B commented Mar 12, 2024

What does this change?

gha-scala-library-release-workflow provides these benefits:

  • better security for release credentials by isolating release phases into separate GitHub Workflow Jobs
  • removes a lot of sbt configuration (gha-scala-library-release-workflow automatically provides sensible defaults)
  • automated version compatibility checking, detecting binary & source incompatibilities, and setting SemVer-compliant version numbers appropriately - meaning that developers no longer have to assign version numbers, and sbt can now reliably detect when dependency eviction could cause runtime errors (see eg Runtime compatibility errors (eg NoSuchMethodError) facia-scala-client#301 !) - and so block those problems at build time.

Changes made according to Configuration section https://github.com/guardian/gha-scala-library-release-workflow/blob/main/docs/configuration.md

How to test

This PR in progress, need to verify sbt file with @rtyley and verify tests.
If all fine then we can try make preview release and test in some of clients using content-entity-model

How can we measure success?

should be able to make release for this library very easily,

@Divs-B Divs-B requested a review from rtyley March 12, 2024 16:27
Copy link
Member

@rtyley rtyley left a comment

Choose a reason for hiding this comment

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

Thanks for adding CI in 92edec6 - note that you also can delete .travis.yml, as that's the legacy of an obsolete CI system!

build.sbt Outdated
.settings(commonSettings)
.settings(npmBetaReleaseTagMaybe)
.settings(artifactProductionSettings)
.settings(npmBetaReleaseTagMaybe)//do we need to remove this?
Copy link
Member

Choose a reason for hiding this comment

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

Yep, we need to remove npmBetaReleaseTagMaybe, along with betaReleaseType & betaReleaseSuffix, etc!

registry-url: https://registry.npmjs.org
- name: Release Typescript to NPM
run: |
sbt "project typescript" "releaseNpm ${{ needs.scala-maven-release.outputs.RELEASE_VERSION }}"
Copy link
Member

Choose a reason for hiding this comment

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

I think the typescript project is called typescriptClasses in this project, for some reason!

lazy val typescriptClasses = (project in file("ts"))

build.sbt Outdated
Comment on lines 66 to 67
packageDoc / publishArtifact := false, //do we need to remove this? also because if root is using "publish / skip := true" instead?
packageSrc / publishArtifact := false, //do we need to remove this? also because if root is using "publish / skip := true" instead?
Copy link
Member

Choose a reason for hiding this comment

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

Interesting - it looks like we've never published the thrift project as an artifact, so actually it would be right to put publish / skip := true on this submodule too!

build.sbt Outdated
// scrooge 21.3.0: Builds are now only supported for Scala 2.12+
// https://twitter.github.io/scrooge/changelog.html#id11
crossScalaVersions := Seq("2.12.11", scalaVersion.value),
crossScalaVersions := Seq("2.12.18", scalaVersion.value),
releaseCrossBuild := true,
Copy link
Member

Choose a reason for hiding this comment

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

This line needs to be moved down into the root project!

Suggested change
releaseCrossBuild := true,

releaseCrossBuild lives with releaseVersion and releaseProcess on the root-level aggregating project, not on the artifact-producing modules!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good spot, I will correct it now 👍

Copy link
Member

@rtyley rtyley left a comment

Choose a reason for hiding this comment

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

I think this all looks good to me, apart from the fact that your tidying-up has now revealed that releaseCrossBuild := true was in the wrong place - once you've moved that I think this should be good to go!

@Divs-B
Copy link
Contributor Author

Divs-B commented Mar 19, 2024

Thanks @rtyley for your help in implementing the release process across all coPip's libraries (this being the last one).
we experiencing efficient results in releasing them.

Will merge this PR to proceed for release testing 👍

@Divs-B Divs-B merged commit 864f9ee into main Mar 19, 2024
1 check passed
@rtyley
Copy link
Member

rtyley commented Mar 19, 2024

Wonderful stuff! Don't forget to kick off a release!

https://github.com/guardian/content-entity/actions/workflows/release.yml

Divs-B added a commit that referenced this pull request Mar 20, 2024
In this PR #32, we specified node-version-file in release.yml file to use .nvmrc file but we didn't add the file and hence got failed on NPM release.
ref: https://github.com/guardian/content-entity/actions/runs/8360348474
rtyley added a commit that referenced this pull request Mar 20, 2024
Comment on lines -150 to +64
packageDoc / publishArtifact := false,
packageSrc / publishArtifact := false,
publish / skip := true,
Copy link
Member

Choose a reason for hiding this comment

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

Ah whoops, I see what happened here! Yep, we didn't want to add publish / skip := true here, because we do want an artifact. I think we probably want the original configuration (packageDoc / publishArtifact := false & packageSrc / publishArtifact := false) to stay as it is - they are only blocking the publishing of src & doc, not attempting to stop all artifacts for this module altogetherm as publishArtifact := false would have been.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @rtyley for confirming, yeah I didn't realise I restricted its publish.
I will revert to old changes on this section now 👍

Divs-B added a commit that referenced this pull request Mar 21, 2024
We have encountered an issue of content-entity-thrift not being released after the work in PR #32 to maven due to error in making its publish to false which is corrected in the PR here #36

To test this we are trying manually version update to the one after which content-entity-thrift didn't get release.
@Divs-B
Copy link
Contributor Author

Divs-B commented Apr 23, 2024

This release is verified and tested.
Checked in content-atom:guardian/content-atom#169 which eventually checked in content-api-models:guardian/content-api-models#246 which at the end tested in one of client using all that is apple-news:https://github.com/guardian/apple-news/pull/331

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

2 participants