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

chore(deps): Update dependencies and Docker images #614

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

reneleonhardt
Copy link

@reneleonhardt reneleonhardt commented Dec 11, 2023

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 reneleonhardt force-pushed the 1.0.x-updates branch 2 times, most recently from 87c59ba to aed7a8a Compare December 11, 2023 20:31
@Neustradamus
Copy link

@reneleonhardt: Nice!

Linked to:

@mp911de
Copy link
Collaborator

mp911de commented Jan 9, 2024

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.

@reneleonhardt
Copy link
Author

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 reneleonhardt force-pushed the 1.0.x-updates branch 2 times, most recently from 81a3b72 to 3b5a7f0 Compare May 26, 2024 13:44
@reneleonhardt reneleonhardt changed the base branch from 1.0.x to main May 26, 2024 13:44
@reneleonhardt
Copy link
Author

reneleonhardt commented Jul 6, 2024

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

@jorsol
Copy link
Member

jorsol commented Jul 15, 2024

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.

@reneleonhardt
Copy link
Author

reneleonhardt commented Jul 15, 2024

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?

@jorsol
Copy link
Member

jorsol commented Jul 15, 2024

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.

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