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

Add Closeable/AutoCloseable on Database & interface #2990

Merged
merged 11 commits into from Jul 20, 2022

Conversation

zorglube
Copy link
Contributor

@zorglube zorglube commented Jun 22, 2022

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Added the interface AutoCloseable to the interfaces DataBase and DatabaseConnection , in order to provid the possibility to use Java try with resources.

@zorglube zorglube marked this pull request as draft June 22, 2022 14:13
@zorglube zorglube changed the title Add Closeable on Database interface Add Closeable/AutoCloseable on Database & interface Jun 22, 2022
@zorglube
Copy link
Contributor Author

This PR is related to this Issue #2974

@zorglube
Copy link
Contributor Author

@kataggart I need a first review please.

@kataggart kataggart added this to To Do in Conditioning++ via automation Jun 24, 2022
@zorglube
Copy link
Contributor Author

zorglube commented Jul 4, 2022

@kataggart I need a first review please.

Please.

@kataggart
Copy link
Contributor

thanks @zorglube it's in the queue to be reviewed.

@kataggart kataggart requested a review from nvoxland July 5, 2022 12:37
@kataggart kataggart mentioned this pull request Jul 5, 2022
@nvoxland
Copy link
Contributor

nvoxland commented Jul 5, 2022

It's looking good so far. Both already have close() methods on them, so it makes sense and won't impact the APIs.

You added the try with resources to the tests, you can add throws Exception to the test method signature vs. just logging the error like you have.

Besides that, I have a vague hope to someday remove the need for a "close()" interface from Database in general to make that class more about being a "dialect" vs. another layer of a connection. But that is already going to be a large enough change that it would be nice to have Autoclosable on Database for now.

@zorglube
Copy link
Contributor Author

zorglube commented Jul 6, 2022

You added the try with resources to the tests, you can add throws Exception to the test method signature vs. just logging the error like you have.

I'll work on that, then ping you back.

@zorglube
Copy link
Contributor Author

@nvoxland : I updated the unit tests. IF it's good for you, do you have any idea of when the code will be merge ?

@nvoxland nvoxland added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Jul 13, 2022
@nvoxland nvoxland marked this pull request as ready for review July 13, 2022 17:58
@nvoxland
Copy link
Contributor

It looks good, @zorglube . I made it a non-draft release and kicked off a build on it to make sure everything is still all passing. After those finish we have a couple other internal review & test steps but I'd imagine it will be in the next release. We generally shoot for a new release every couple weeks and just had one recently, so I'd guess it will be around then.

@nvoxland
Copy link
Contributor

Code review and test results:

Things to be aware of:

  • Only "shipped" change is adding the Closable interface to Database and DatabaseConnection. Both of which already defined the close() method so no real code impact beyond that extra marker interface.
  • The rest of the changes are fixes to tests to avoid linters complaining about "not closing autoclosable objects"

Things to worry about:

  • Nothing

@github-actions
Copy link

Unit Test Results

  4 608 files  ±0    4 608 suites  ±0   33m 23s ⏱️ - 3m 4s
  4 589 tests ±0    4 375 ✔️ ±0     214 💤 ±0  0 ±0 
54 240 runs  ±0  49 232 ✔️ ±0  5 008 💤 ±0  0 ±0 

Results for commit ad612b8. ± Comparison against base commit bcd2d53.

Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

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

  • Fix enhances the existing 'close' method in Database and DatabaseConnection to add the ability to use Java try with resources .
  • This PR is "noisy" with a lot of changes that came in from a merge.
    • Many changes normalize our usage of the changeset v. change set terminology.
    • Many other changes are related to using XSD latest & warning if XSD versions are mismatched.
    • Nothing to worry about, just noting that is why the number of changed files is large for a very focused change to the code.
  • No additional testing necessary.

APPROVED

Passing Functional Tests
Passing Test Harness Execution

@nvoxland nvoxland changed the base branch from master to 1_9 July 20, 2022 14:41
@nvoxland nvoxland changed the base branch from 1_9 to master July 20, 2022 14:41
@nvoxland nvoxland merged commit c72e682 into liquibase:master Jul 20, 2022
Conditioning++ automation moved this from To Do to Done Jul 20, 2022
@zorglube
Copy link
Contributor Author

Thanks guys! ☺️

@kataggart kataggart added this to the NEXT milestone Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autocandidate daily_review SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions TypeEnhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add Closeable/AutoCloseable to the interface liquibase.database.Database
5 participants