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

Implement gha-scala-library-release-workflow release process. #402

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

Divs-B
Copy link
Contributor

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

What does this change?

This Library had GHA recently but now introducing more secured, less code and easy deployable way of automised workflow that is gha-scala-library-release-workflow

ref:https://github.com/guardian/gha-scala-library-release-workflow/blob/main/docs/configuration.md

How to test

Tests are passing locally.

Need to test further by releasing snapshot release once the PR is reviewed, finalised and merged.

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.

Note that you will need to fix version.sbt in this PR as well, as it's currently out of date - there has already been release 20.0.0, but they forgot to update version.sbt to reflect that...!

ThisBuild / version := "19.4.2-SNAPSHOT"

@Divs-B
Copy link
Contributor Author

Divs-B commented Jan 12, 2024

Thanks for letting me know @rtyley , I will update version.sbt with latest version.

build.sbt Outdated
Comment on lines 29 to 30
Compile / sources := Seq.empty,
Test / sources := Seq.empty,
Copy link
Member

Choose a reason for hiding this comment

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

I'm fairly sure these two lines can be deleted too!

Suggested change
Compile / sources := Seq.empty,
Test / sources := Seq.empty,

build.sbt Outdated
checkSnapshotDependencies,
inquireVersions,
runClean,
runTest,
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid you'll need to remove runTest for the time being, as, like in the facia-scala-client PR, the content-api-scala-client tests require an additional credential (CAPI_TEST_KEY) that is difficult to pass down through the reusuable release workflow.

Suggested change
runTest,

//
// val awsDevs =
// List( Developer(id="tomrf1", name="Tom Forbes", email="", url=url("https://github.com/tomrf1"))
// )
Copy link
Member

@rtyley rtyley Jan 12, 2024

Choose a reason for hiding this comment

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

It turns out this entire file (Metadata.scala) can be deleted!

It's full of metadata that is only for serving the Maven Central metadata requirements - but that information is now provided by gha-scala-library-release-workflow in this one line! (guardian/gha-scala-library-release-workflow#4 has a little more info about this)

The only 2 bits that are still needed are:

  • licenses := Seq(License.Apache2) - can move up into the artifact-generating modules in build.sbt
  • val ghProject = "content-api-client" - can just be up near the top of build.sbt

...but apart from that, anything that's using the Metadata class in build.sbt just needs to drop the reference to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair point 👍

@@ -1,7 +1,7 @@
import sbt._

object Dependencies {
val scalaVersions = Seq("2.12.10", "2.13.1")
val scalaVersions = Seq("2.12.17", "2.13.12")
Copy link
Member

Choose a reason for hiding this comment

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

There's a slightly newer version of Scala 2.12 we can use!

Suggested change
val scalaVersions = Seq("2.12.17", "2.13.12")
val scalaVersions = Seq("2.12.18", "2.13.12")

@@ -1 +1 @@
ThisBuild / version := "19.4.2-SNAPSHOT"
ThisBuild / version := "20.0.1-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

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

Perfect! Yep, this is good, we've already had release 20.0.0 so now we need to be setting ourselves up for 20.0.1!

build.sbt Outdated
Comment on lines 53 to 55
// we apply versionSettingsMaybe to aws too because this project is always released in isolation
// from the others - and we _might_ want to put out a snapshot or beta for that (TBC).
// Bear this in mind if we begin releasing aws in sync with the others
Copy link
Member

@rtyley rtyley Jan 12, 2024

Choose a reason for hiding this comment

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

Note that we should delete this comment too, as part of this PR, because it talks about versionSettingsMaybe, and we're deleting that on the line below.

Suggested change
// we apply versionSettingsMaybe to aws too because this project is always released in isolation
// from the others - and we _might_ want to put out a snapshot or beta for that (TBC).
// Bear this in mind if we begin releasing aws in sync with the others
// we apply versionSettingsMaybe to aws too because this project is always released in isolation
// from the others - and we _might_ want to put out a snapshot or beta for that (TBC).
// Bear this in mind if we begin releasing aws in sync with the others

The comment was added with PR #324, and it makes an interesting point - that there essentially two separate groups of libraries in this repository, and they don't actually depend on each other - content-api-client-aws doesn't depend on content-api-client, content-api-client-default, etc, and they don't depend on it. They have different version numbers (content-api-client-aws is only at version 0.7), and I think this leaves us with two possible courses of action, which we need to choose from:

  • Synchronise them up and release them together, sharing a version number
  • Separate content-api-client-aws out into its own GitHub repository, and have a separate release setup for that - right now, I think this is the one that makes most sense to me.

Divs-B added a commit that referenced this pull request Jan 17, 2024
The reason for extracting the `aws` project is that it is not dependent on `content-api-scala-client` and was also released separately in the past, so it has different version numbers.

further reference is here: #402 (comment)

ref:https://github.com/guardian/content-api-client-aws
@Divs-B Divs-B force-pushed the db/implement-gh-scala-release-process branch from b90ab8f to 4f943f3 Compare January 17, 2024 16:40
@Divs-B Divs-B marked this pull request as ready for review January 17, 2024 16:52
@Divs-B Divs-B requested a review from a team as a code owner January 17, 2024 16:52
Comment on lines -45 to +42
javacOptions ++= Seq("-source", "1.8", "-target", "1.8"),
scalacOptions ++= Seq("-deprecation", "-unchecked"),
scalacOptions ++= Seq("-deprecation", "-unchecked", "-release:8"),
Copy link
Member

Choose a reason for hiding this comment

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

Using -release:8 will include setting "-target", "1.8" for the Java compiler, and we don't need "-source", "1.8" as we are not using any Java source files!

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.

Nice work!

@Divs-B
Copy link
Contributor Author

Divs-B commented Jan 17, 2024

Thanks @rtyley for pairing and all help.

@Divs-B Divs-B merged commit f8ce129 into main Jan 17, 2024
2 checks passed
Divs-B added a commit that referenced this pull request Jan 17, 2024
This will fix the release process which was failing because it was searching for artifact group id.

Error ref:https://github.com/guardian/content-api-scala-client/actions/runs/7559137372/job/20582282903#step:4:211

We forgot these settings in prev PR: #402
rtyley added a commit that referenced this pull request Jun 11, 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.

2 participants