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

#118: DB cleanup and IT tests addition to CI #119

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

lsulak
Copy link
Collaborator

@lsulak lsulak commented Mar 25, 2024

Preparation for #118 - I don't really want to do all the DB setup for tests manually, it's too much effort. I used Flyway to automate it. It's not ideal - the actual data insertions and cleanups after each test - but it's better than it was and we can improve it on the next increment.

What was done:

--

Relates to #118
Relates to #116
Relates to #121
Relates to #19

Copy link

JaCoCo code coverage report - scala 2.13.12

Total Project Coverage 27.94% 🍏

There is no coverage information present for the Files changed

Copy link

JaCoCo code coverage report - scala 2.12.17

Total Project Coverage 27.05% 🍏

There is no coverage information present for the Files changed

salamonpavel
salamonpavel previously approved these changes Apr 22, 2024
Copy link
Contributor

@salamonpavel salamonpavel left a comment

Choose a reason for hiding this comment

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

Good job, this was definitely missing. Let's address this also in other projects.

database/README.md Outdated Show resolved Hide resolved
flywayPassword := FlywayConfiguration.flywayPassword
flywayLocations := FlywayConfiguration.flywayLocations
flywaySqlMigrationSuffixes := FlywayConfiguration.flywaySqlMigrationSuffixes
libraryDependencies ++= flywayDependencies
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 it adds to the global dependencies, doesn't it? I think flyway dependencies shouldn't be a generic dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is what @salamonpavel was dealing with in the past and there was no way how to circumvent this (I know :( ) by extracting / moving elsewhere. Anyway, it's part of the build, i.e. general dependency: https://github.com/AbsaOSS/atum-service/blob/master/build.sbt#L53-L59

(remember, Flyway is temporary anyway)

Copy link
Contributor

@Zejnilovic Zejnilovic Apr 26, 2024

Choose a reason for hiding this comment

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

I agree with @benedeki

lazy val modulNeedingFlyaway = project
  .enablePlugins(FlywayPlugin)
  .settings(
    flywayUrl := FlywayConfiguration.flywayUrl
    flywayUser := FlywayConfiguration.flywayUser
    flywayPassword := FlywayConfiguration.flywayPassword
    flywayLocations := FlywayConfiguration.flywayLocations
    flywaySqlMigrationSuffixes := FlywayConfiguration.flywaySqlMigrationSuffixes
    libraryDependencies ++= flywayDependencies
  )

and then run it like sbt modulNeedingFlyaway/flywayMigrate

Are we saying there is an issue with this @lsulak ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we redefine the flywayMigrate to behave like sbt modulNeedingFlyaway/flywayMigrate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like this addCommandAlias("flywayMigrate", "modulNeedingFlyaway/flywayMigrate")?

Copy link
Collaborator Author

@lsulak lsulak Apr 26, 2024

Choose a reason for hiding this comment

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

Let me try, thanks @Zejnilovic. To be fair I haven't been playing with this one as my primary objective here was to actually have an IT suite embedded in our CI & I borrowed what was done in Atum Service - but I agree that we can improve this mechanism in build.sbt

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why it got approved, not blocking the whole thing 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tested, it works very nicely, thank you Sasa! #124

build.sbt Show resolved Hide resolved
…s a subset of what we have anyway, and it's completely ignored
Copy link
Contributor

@benedeki benedeki left a comment

Choose a reason for hiding this comment

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

Well, except the Flyway dependency I think it's OK. That one can be addressed later.

@lsulak lsulak merged commit c06a209 into master Apr 26, 2024
9 checks passed
@lsulak lsulak deleted the feature/118-db-function-cleanup-pre-118 branch April 26, 2024 09:01
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.

4 participants