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

migrate to ZIO 2 #38

Merged
merged 9 commits into from
Jul 13, 2022
Merged

migrate to ZIO 2 #38

merged 9 commits into from
Jul 13, 2022

Conversation

alterationx10
Copy link
Contributor

To address #36 , I followed the Migration Guide, and it seems like it all compiles 😄

I also bumped the sbt version, as the one listed doesn't seem to play well with jdk 18

// https://github.com/google/tink/issues/72
randomRef <- FiberRef.make[JSecureRandom](new JSecureRandom())
val live: Layer[NoSuchAlgorithmException, SecureRandom] = ZLayer {
ZIO.scoped {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the appropriate use of Scoped? Wasn't sure for this case, but seems ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

No this will release the scope at the end of the creation of the layer and leak out the released fiber ref.

You need to use ZLayer.scoped { ... } which creates a Scope for the lifecycle of the layer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Oh yeah. I'll get this updated!

@alterationx10
Copy link
Contributor Author

I guess there doesn't seem to be any CI tests run? "Works on my machine" if it makes anyone feel better 🤣

[info] 96 tests passed. 0 tests failed. 0 tests ignored.

@vigoo
Copy link
Contributor

vigoo commented Jul 6, 2022

I guess there doesn't seem to be any CI tests run? "Works on my machine" if it makes anyone feel better 🤣

[info] 96 tests passed. 0 tests failed. 0 tests ignored.

As first contributor we have to explicitly approve CI runs for you, after this is merged it will no longer be necessary :)

def encrypt(plainText: Chunk[Byte], key: PublicKey): RIO[Has[HybridEncryption], CipherText[Chunk[Byte]]] =
ZIO.accessM(_.get.encrypt(plainText, key))
def encrypt(plainText: Chunk[Byte], key: PublicKey): RIO[HybridEncryption, CipherText[Chunk[Byte]]] =
ZIO.environmentWithZIO(_.get.encrypt(plainText, key))
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be ZIO.serviceWithZIO(_.encrypt(plainText, key))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scalafix from the migration did all of these. I noticed it changed .accessM to environmentWithZIO, but it didn't updated anything that was .access. I noticed this behavior with a work project I was testing migrations on for an earlier RC. Is this something that might need to be updated in the scalafix rule?

Either way, it probably makes more sense to make them . serviceWithZIO - I'll update them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Didn't notice the .get right away.

for {
key <- KeysetManager.generateNewAsymmetric(algorithm)
ciphertext1 <- HybridEncryption.encrypt(m, key.publicKeyset)
ciphertext2 <- HybridEncryption.encrypt(m, key.publicKeyset)
} yield assert(ciphertext1)(not(equalTo(ciphertext2)))
} yield assertTrue(!(ciphertext1 == ciphertext2))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a != b is easier to read :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 I agree. I was mostly just following the auto-suggestion from the zio plugin while I was fixing these specs. I'll update these too.

ZIO 2 was built against 3.1.2, and this is using (incompatible) 3.0.1.

I updated the scala matrix to match that of the core ZIO library.

I also tweaked BuildHelper to have ScalaDotty be versions("3") instead of "3.0",
along with relevant ci.yml changes
SilencerVersion => 1.7.8 (matching core zio lib)
sbt-scalafix => 0.10.0 (mathcing core zio lib)
kind-projector => 0.13.2 (bumped to latest)
@alterationx10
Copy link
Contributor Author

I made a slight change to the ci.yml file

ZIO 2 was (cross) built against 3.1.2, and this is using (incompatible) 3.0.1.

I updated the Scala matrix to match that of the current core ZIO library.

The version bump to Scala 2.13.8 meant updating a couple dependencies as well:

SilencerVersion => 1.7.8 (matching core zio lib)
sbt-scalafix => 0.10.0 (mathcing core zio lib)
kind-projector => 0.13.2 (bumped to latest)

@alterationx10 alterationx10 requested a review from vigoo July 7, 2022 02:19
@vigoo vigoo merged commit 23de4b4 into zio:main Jul 13, 2022
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