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

Change value modifier to make StandardLockService extendable for future extension #2785

Merged

Conversation

Cliftonz
Copy link
Contributor

This pull request changes the modifiers of standard lock service to protected for the case where only certain parts of the lock service need to be rewritten.

An example is this pr in the Keyspace extension.
liquibase/liquibase-amazon-keyspaces#8

…xtensions that may need custom lock service like keyspace
@kataggart kataggart added this to To Do in Conditioning++ via automation Apr 22, 2022
@nvoxland nvoxland changed the base branch from master to 1_9 May 21, 2022 05:48
@nvoxland nvoxland changed the base branch from 1_9 to master May 21, 2022 05:49
@nvoxland nvoxland added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label May 21, 2022
Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

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

The changes make sense. I held off a bit on it since we're planning some level of refactoring around StandardLockService at some point (including to make it more extensible) and making the internal fields protected will make us have to work harder to preserve them if/when we do the refactoring.

But, I'm not sure the timeline on that and so don't want to continue to block this on the off chance something changes then.

@github-actions
Copy link

github-actions bot commented May 21, 2022

Unit Test Results

  4 512 files    4 512 suites   33m 21s ⏱️
  4 419 tests   4 205 ✔️    214 💤 0
52 308 runs  47 300 ✔️ 5 008 💤 0

Results for commit f2f2ef3.

♻️ This comment has been updated with latest results.

@Cliftonz
Copy link
Contributor Author

@nvoxland It looks like the CI thinks that this pr is not safe to build...

@Cliftonz Cliftonz requested a review from nvoxland May 24, 2022 17:44
@nvoxland nvoxland added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels May 25, 2022
@Cliftonz
Copy link
Contributor Author

Cliftonz commented May 25, 2022

@nvoxland What do I need to do to make Liquibase pro stable? or is this even needed for this to merge in?

@XDelphiGrl
Copy link
Contributor

@nvoxland What do I need to do to make Liquibase pro stable? or is this even needed for this to merge in?

Hello, @Cliftonz! There is nothing you can do from your side to help with the stability of the Liquibase Pro build. I am kicking off the functional and test harness test suites manually; when those pass, I'll mark this as ready to merge. Thank you for the PR and for your patience!

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.

  • This fix has no impact on the usual CLI workflows (i.e., only extension developers will be impacted (in a good way!)).
  • No additional testing required.

APPROVED

Functional Test Results
Test Harness Results

@nvoxland nvoxland merged commit f0df141 into liquibase:master May 25, 2022
Conditioning++ automation moved this from To Do to Done May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autocandidate DBKeyspace Extensions 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.

None yet

5 participants