r2dbc-postgresql icon indicating copy to clipboard operation
r2dbc-postgresql copied to clipboard

chore(deps): Update dependencies and Docker images

Open reneleonhardt opened this issue 1 year ago • 7 comments

Issue description

  • Update dependencies, plugins and Maven (Wrapper)
  • Update Docker images and switch to alpine
  • Use SCRAM authentication instead of md5 (default since Postgres 14)
  • Testcontainers only use JUnit Jupiter
  • Run tests with UTF-8 encoding and disable logging
  • Fix test.bash
  • Let Dependabot update Maven dependencies and GitHub Actions weekly
  • Build with JDK 17 for Java 8

Additional context

There were no tests for PgPool, I just updated the Docker image.

reneleonhardt avatar Dec 11 '23 19:12 reneleonhardt

@reneleonhardt: Nice!

Linked to:

  • https://github.com/scram-sasl/info/issues/1

Neustradamus avatar Jan 06 '24 20:01 Neustradamus

Thanks for looking into this. The way the changes are arranged makes it impossible to backport these to the currently maintained version 1.0.x. A lot of these changes make sense and are interesting though.

mp911de avatar Jan 09 '24 08:01 mp911de

Thanks for looking into this. The way the changes are arranged makes it impossible to backport these to the currently maintained version 1.0.x. A lot of these changes make sense and are interesting though.

I can rebase onto main if that's what you mean by backport 😅

reneleonhardt avatar May 26 '24 11:05 reneleonhardt

@mp911de Can you test this branch yourself and clarify what would be needed to merge updates?

reneleonhardt avatar Jul 06 '24 04:07 reneleonhardt

Hi @reneleonhardt , I think what Mark wants is something like atomic commits, you need to rebase with main and send commits that are independent of each other, to give an example you need to separate the changes in individual commits like:

  • Update Maven Wrapper to 3.3.2 and Maven to 3.9.8
  • Add Dependabot configuration
  • Update maven plugins
  • Update dependencies
  • Testcontainers only use JUnit Jupiter
  • Run tests with UTF-8 encoding and disable logging
  • Build with JDK 17 for Java 8

More or less essentially the list you provide in the description, but each should be an individual commit rebased on top of main.

BTW, normally when there are unrelated changes, the best thing is to send multiple PRs, but this also depends on the maintainer of the project if it's ok to have a single PR like this.

jorsol avatar Jul 15 '24 16:07 jorsol

Unrelated changes?

Updates

  • Update Maven Wrapper to 3.3.2 and Maven to 3.9.8
  • Add Dependabot configuration
  • Update maven plugins
  • Update dependencies
  • Build with JDK 17 for Java 8

Improve Tests

  • Testcontainers only use JUnit Jupiter
  • Run tests with UTF-8 encoding and disable logging

There will be a myriad of conflicts when any other PR get's merged before all of them are merged into main, what is the advantage of 10 commits changing 1 line over 1 PR changing 10 lines in the same file?

reneleonhardt avatar Jul 15 '24 16:07 reneleonhardt

Unrelated changes?

Updates

* Update Maven Wrapper to 3.3.2 and Maven to 3.9.8

* Add Dependabot configuration

* Update maven plugins

* Update dependencies

* Build with JDK 17 for Java 8

Improve Tests

* Testcontainers only use JUnit Jupiter

* Run tests with UTF-8 encoding and disable logging

There will be a myriad of conflicts when any other PR get's merged before all of them are merged into main, what is the advantage of 10 commits changing 1 line over 1 PR changing 10 lines in the same file?

Yes, unrelated changes, updating Maven Wrapper is not related and does not affect updating the dependabot configuration, and updating the tests is not related to building with JDK 17, and so on...

I'm not a maintainer here but is just a good practice to make commits atomic, what this means is that if you update one part of the code that updates dependencies, that's one commit, if you update SCRAM code (including tests) that's another commit, in general changes that can live "independent" of each other should be different commits, there is no global "Updates" or "Improve Tests" category.

What is the advantage of having 10 commits instead of a single one with all the changes mixed? Is simply maintainability, if one change (commit) turns out to be broken it can be reverted that single commit without affecting all others, another advantage is that if multiple branches need to be "supported" is easiest to cherry-pick one commit and apply it to that branch, maybe the Maven Wrapper update make sense to backport to an older branch, but maybe the SCRAM changes do not, so having them in a single commit makes it impossible to backport the changes to that branch.

jorsol avatar Jul 15 '24 18:07 jorsol