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

refactor: migrate to zio-json + update all #438

Closed

Conversation

ThijsBroersen
Copy link

Why circe if we have zio-json? 😏

I think main is broken because some PR's got merged which did not pass CI. Like circe-yaml 0.16.0 which is not available for Scala 2.12 anymore.

This PR updates all dependencies and migrates to zio-json. I see some other areas where improvements can be made but will target that in separate PR's.

I ran sbt prepare as in the Contributing guide but not sbt testPlugin that seems broken. I could not find any reference to the testPlugin task. I also could not find any tests in the repo... how is this project tested? Manually?

With zio-json I created codecs (both encoders and decoders) so perhaps validations can now also be done by parsing the yaml and decoding back to the model and then create a set of rules for it. Perhaps even allow manual tweaks and on each new generate-run first load the existing flow and then update this flow.

@ThijsBroersen ThijsBroersen changed the title refactor: migrate to zio-json refactor: migrate to zio-json + update all Oct 17, 2024
@ThijsBroersen
Copy link
Author

ThijsBroersen commented Oct 17, 2024

The PR is btw breaking because it changes the model so the codecs become easier because they match the GHA model more closely. Any opinions?
I think it helps to migrate and not have to remember different views on the same thing.

--update--
I restored the existing model and added a new one. With toNative methods to convert. I tested it on a few zio projects and the required changes are quite small.

@@ -190,7 +190,7 @@ trait ScalaCompilerSettings {
)
} else Seq.empty
},
Test / parallelExecution := scalaBinaryVersion.value != "3",
Test / parallelExecution := scalaBinaryVersion.value != "3", // why not parallel execution for Scala 3?
Copy link
Author

Choose a reason for hiding this comment

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

Discovered this setting.. why was this set? I would prefer to not have this set.

@@ -233,8 +233,7 @@ trait ScalaCompilerSettings {
libraryDependencies ++= Seq(
"dev.zio" %%% "zio-test" % ZioSbtEcosystemPlugin.autoImport.zioVersion.value % Test,
"dev.zio" %%% "zio-test-sbt" % ZioSbtEcosystemPlugin.autoImport.zioVersion.value % Test
),
testFrameworks += new TestFramework("zio.test.sbt.ZTestFramework")
)
Copy link
Author

Choose a reason for hiding this comment

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

For quite some time ZTestFramework is added to testFrameworks in sbt by default.

val Scala212 = "2.12.20"
val Scala213 = "2.13.15"
val Scala3 = "3.3.4"
val zio = "2.1.9"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val zio = "2.1.9"
val zio = "2.1.11"

libraryDependencies += "dev.zio" %% "zio" % "2.1.8"
libraryDependencies += "io.circe" %% "circe-yaml" % "0.16.0"
libraryDependencies += "org.snakeyaml" % "snakeyaml-engine" % "2.8"
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
libraryDependencies += "dev.zio" %% "zio" % "2.1.11"

@@ -1,2 +1,3 @@
libraryDependencies += "dev.zio" %% "zio" % "2.1.8"
libraryDependencies += "io.circe" %% "circe-yaml" % "0.16.0"
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
libraryDependencies += "dev.zio" %% "zio" % "2.1.11"

libraryDependencies += "org.snakeyaml" % "snakeyaml-engine" % "2.7"
libraryDependencies += "dev.zio" %% "zio" % "2.1.8"
libraryDependencies += "io.circe" %% "circe-yaml" % "0.16.0"
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
libraryDependencies += "dev.zio" %% "zio" % "2.1.11"


val zioVersion = "2.0.21"
val zioVersion = "2.1.9"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val zioVersion = "2.1.9"
val zioVersion = "2.1.11"

@@ -1,2 +1,3 @@
libraryDependencies += "dev.zio" %% "zio" % "2.1.8"
libraryDependencies += "io.circe" %% "circe-yaml" % "0.16.0"
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libraryDependencies += "dev.zio" %% "zio" % "2.1.9"
libraryDependencies += "dev.zio" %% "zio" % "2.1.11"


// The original code of the githubactions package was originally copied from the zio-aws-codegen project:
// https://github.com/zio/zio-aws/tree/master/zio-aws-codegen/src/main/scala/zio/aws/codegen/githubactions
object ScalaWorkflow {
Copy link
Member

@guizmaii guizmaii Oct 19, 2024

Choose a reason for hiding this comment

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

What's the need to rewrite this code from scratch?

Copy link
Author

Choose a reason for hiding this comment

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

I added the new solution next to the existing. But I think in the end result the old model and workflow are not used anymore in the other modules.
Perhaps the question should be: should we use the new model, stick with the old or somehow support both?
My reasons for the new model is that it is closer to writing direct GHA yaml and less of a cognitive load (no need to learn another DSL). It also simplifies the codecs because we reduce the amount of codec adjustments or required DTO's.

Copy link
Author

Choose a reason for hiding this comment

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

I would love to contribute some more but I really think a model which is as much 1:1 with GHA model as possible, helps to build better workflows. It is predictable and the codecs are easier.

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