Skip to content

Update dependencies (avro4s update) #1183

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

Merged
merged 6 commits into from
May 4, 2021
Merged

Conversation

alejandrohdezma
Copy link
Contributor

@alejandrohdezma alejandrohdezma commented Apr 27, 2021

What does this change do?

Updates all the updatable dependencies to their latest versions, except the ones that involve updating to cats-effect 3.0:

  • Update Scala version (this involved a bunch of nowarn annotations).
  • Update cats-effect to 2.5.0.
  • Update organization for log4cats (is now under the Typelevel organization).
  • Update avro4s to 4.0.7 (this involved a few removed parameters and Decoder and Encoder classes now including the SchemaFor (instead of handling it implicitly).

Checklist

  • Reviewed the diff to look for typos, println and format errors.
  • Updated the docs accordingly.

fedefernandez
fedefernandez previously approved these changes Apr 27, 2021
Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@alejandrohdezma
Copy link
Contributor Author

Have the error in the build happened in the past? I can't see why it is happening

@juanpedromoreno
Copy link
Member

@alejandrohdezma You could try downgrading the Scala version. I'm not fully sure, but that error is familiar to me 🤔

@juanpedromoreno juanpedromoreno mentioned this pull request Apr 27, 2021
2 tasks
@juanpedromoreno
Copy link
Member

Yes, it seems that's the problem. That scala version causes scoverage issues like this. It's not exactly that, but I believe it's related.

I checked it in this PR: #1184, where the error is different (Avro test failing, apparently).

Thanks @alejandrohdezma !

franciscodr
franciscodr previously approved these changes Apr 27, 2021
Copy link
Contributor

@franciscodr franciscodr left a comment

Choose a reason for hiding this comment

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

Great work! Thanks, @alejandrohdezma

@alejandrohdezma alejandrohdezma force-pushed the update/update-dependencies branch from 18a1989 to b781c70 Compare April 28, 2021 09:29
@alejandrohdezma alejandrohdezma force-pushed the update/update-dependencies branch from b781c70 to 82c3557 Compare April 28, 2021 09:34
@alejandrohdezma
Copy link
Contributor Author

@juanpedromoreno downgrading Scala version again seemed to fixed the issue, but as you noticed the RPCTests failed :(

@alejandrohdezma
Copy link
Contributor Author

alejandrohdezma commented Apr 28, 2021

Can someone that knows their way around these tests help me troubleshoot it? There are so many logs that I'm a bit lost

This test fails because of this update in `avro4s`: sksamuel/avro4s#594.

It is a corner case that will occur only if all the following applies:
- The user is generating IDLs from Scala code (it can never happen the other way around).
- A field in a request/response is using a coproduct.
- Said coproduct is using a generic type.
- Said field has a default value.

If all the previous checks, the generated schema will not contain the default value.
@alejandrohdezma
Copy link
Contributor Author

@franciscodr managed to find the cause of the errors and the "solution". As said in a232ee2, the only solution was to ignore the failing test.

This test fails because of this update in avro4s: sksamuel/avro4s#594.

It is a corner case that will occur only if all the following applies:

  • The user is generating IDLs from Scala code (it can never happen the other way around).
  • A field in a request/response is using a Coproduct.
  • Said Coproduct is using a generic type.
  • Said field has a default value.

If all the previous checks, the generated schema will not contain the default value.

We discussed it and concluded that it is such a small case that we are safe to go down this road.

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #1183 (280101b) into main (b9ca5d6) will decrease coverage by 0.16%.
The diff coverage is 76.92%.

❗ Current head 280101b differs from pull request most recent head ad1a0ea. Consider uploading reports for the commit ad1a0ea to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1183      +/-   ##
==========================================
- Coverage   84.98%   84.81%   -0.17%     
==========================================
  Files          59       59              
  Lines         839      843       +4     
  Branches        1        1              
==========================================
+ Hits          713      715       +2     
- Misses        126      128       +2     
Impacted Files Coverage Δ
...scala/higherkindness/mu/kafka/ConsumerStream.scala 100.00% <ø> (ø)
...scala/higherkindness/mu/kafka/ProducerStream.scala 85.71% <ø> (ø)
...higherkindness/mu/rpc/internal/encoders/avro.scala 80.21% <75.00%> (-1.39%) ⬇️
...cala/higherkindness/mu/format/AvroWithSchema.scala 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9ca5d6...ad1a0ea. Read the comment docs.

@alejandrohdezma alejandrohdezma added dependency-update A dependency version update and removed scala-steward Dependency auto updates created by scala-steward labels May 4, 2021
@alejandrohdezma alejandrohdezma merged commit 057b7d9 into main May 4, 2021
@alejandrohdezma alejandrohdezma deleted the update/update-dependencies branch May 4, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency-update A dependency version update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants