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

introduce zio into atum server #150

Merged
merged 41 commits into from
Feb 1, 2024
Merged

introduce zio into atum server #150

merged 41 commits into from
Feb 1, 2024

Conversation

salamonpavel
Copy link
Collaborator

@salamonpavel salamonpavel commented Jan 22, 2024

ZIO framework was introduced to Atum server module as a replacement of Spring. Endpoints were defined using Tapir since available interpreters can generate Http routes for Http4s server backend and Swagger docs from Tapir endpoints. Simple database integration tests were introduced to verify that Doobie based function objects work as expected as they replaced Slick based ones. Atum server module's target Scala version upgraded to 2.13. Jacoco plugin upgraded to 1.6.1 as the previous version wasn't compatible with upgraded github runners anymore. Missing test coverage partially addressed.

! Classes currently not covered by test cases (mainly endpoints) will be tested as part of #152 when we upgrade target Java version #151 !

Closes #147
Closes #148
Closes #149

@salamonpavel salamonpavel added Server Issues touching the server part of the project work in progress Work on this item is not yet finished (mainly intended for PRs) labels Jan 22, 2024
@salamonpavel salamonpavel self-assigned this Jan 22, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Jan 25, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Jan 25, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Jan 25, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Jan 25, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Jan 25, 2024
@salamonpavel salamonpavel removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jan 25, 2024
project/Dependencies.scala Outdated Show resolved Hide resolved
lsulak
lsulak previously approved these changes Jan 31, 2024
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

Please update the main Readme in the repo as well, it still contains a Spring section

@lsulak lsulak self-requested a review January 31, 2024 16:26
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Feb 1, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Feb 1, 2024
@AbsaOSS AbsaOSS deleted a comment from github-actions bot Feb 1, 2024
Copy link

github-actions bot commented Feb 1, 2024

JaCoCo agent module code coverage report - spark:2 - scala 2.12.18

Overall Project NaN% NaN% 🍏

There is no coverage information present for the Files changed

Copy link

github-actions bot commented Feb 1, 2024

JaCoCo server module code coverage report - scala 2.13.11

Overall Project 62.13% -286.79%
Files changed 37.33%

File Coverage
PostgresDatabaseProvider.scala 100% 🍏
Runs.scala 100% 🍏
CheckpointRepository.scala 100% 🍏
PartitioningRepository.scala 100% 🍏
PartitioningForDB.scala 100% -390%
PostgresConfig.scala 100% -2139.29%
AppError.scala 100% -1400%
CheckpointController.scala 100% 🍏
PartitioningController.scala 100% 🍏
CheckpointService.scala 100% 🍏
PartitioningService.scala 100% 🍏
WriteCheckpoint.scala 76.56% -28.13% 🍏
CreatePartitioningIfNotExists.scala 68.93% -38.57% 🍏
ErrorResponse.scala 40% -1382.5%
DoobieImplicits.scala 33.33% -693.94%
TransactorProvider.scala 0% -1150%
PlayJsonImplicits.scala 0% -930.16%
Server.scala 0% -162.65%
Main.scala 0% -1410%
Constants.scala 0% -41.67%

Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

Good to go in my view!

@salamonpavel salamonpavel merged commit 33edba5 into master Feb 1, 2024
7 of 9 checks passed
@salamonpavel salamonpavel deleted the feature/147-zio branch February 1, 2024 14:03
Copy link

@yruslan yruslan left a comment

Choose a reason for hiding this comment

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

Hey, I didn't make in time before the merge 😄

This is very cool and very educating form me!

Just have one question of curiosity.

Comment on lines +27 to +31
trait CheckpointController {
def createCheckpoint(checkpointDTO: CheckpointDTO): IO[ErrorResponse, CheckpointDTO]
}

class CheckpointControllerImpl(checkpointService: CheckpointService) extends CheckpointController {
Copy link

Choose a reason for hiding this comment

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

Is it a usual ZIO pattern to have both the trait and the class in the same file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yruslan
I haven't encountered any specific guidelines related to ZIO for this particular matter. Personally, I lean towards keeping interfaces or traits in separate files. However, I've noticed that this practice isn't consistently followed in the Atum project. Perhaps having these interfaces in the same files might better illustrate how to construct ZIO services. We can definitely reconsider this approach in the future, especially if we bring in other implementations. I'm curious to hear your thoughts on this topic. What's your perspective?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually funny. I saw this approach first and Hector, who did it in Atum Agent.
I am personally not that big of a fan either, but didn't consider it serious enough at that stage of development. Then I saw you @salamonpavel using it and thought, it's one of the conventions out there. So I didn't bring it up anymore.

if we agree we prefer the style of having them in separate files, as it seems, let's do it 😄

Copy link

Choose a reason for hiding this comment

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

I'm on the same page. Having them separately is probably just a habit for me over some time. Definitely not a big deal in Scala, and if it better illustrates how ZIO works it is a good reason. For me class name having the same name as train/class inside is more for 2 reasons:

  • It also going to match .class files in target
  • IntelliJ idea does not place good icon on files for which class and file name do not match 😆 :

Screenshot 2024-02-09 at 12 44 54

So overall no strong opinions, especially for Atum, completely up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Server Issues touching the server part of the project
Projects
None yet
5 participants