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

Make liquibase.snapshot.ResultSetCache an extensible class. #2087

Merged
merged 2 commits into from Aug 31, 2022

Conversation

breglerj
Copy link
Contributor

@breglerj breglerj commented Sep 10, 2021

This is useful in case an extension has to override
liquibase.snapshot.jvm.UniqueConstraintSnapshotGenerator#listConstraints.

Environment

Liquibase Version:
4.5.0

Liquibase Integration & Version: <Pick one: CLI, maven, gradle, spring boot, servlet, etc.>
CLI

Liquibase Extension(s) & Version:
liquibase-hanadb

Database Vendor & Version:
SAP HANA

Operating System Type & Version:

Pull Request Type

  • Bug fix (non-breaking change which fixes an issue.)
  • Enhancement/New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

A clear and concise description of the issue being addressed. Additional guidance here.

  • Describe the actual problematic behavior.
  • Ensure private information is redacted.

see liquibase/liquibase-hanadb#69

Steps To Reproduce

List the steps to reproduce the behavior.

  • Please be precise and ensure private information is redacted
  • Include things like
    • Files used - sql scripts, changelog file(s), property file(s), config files, POM Files
    • Exact commands used - CLI, maven, gradle, spring boot, servlet, etc.

see liquibase/liquibase-hanadb#69

Actual Behavior

A clear and concise description of what happens in the software before this pull request.

  • Include console output if relevant
  • Include log files if available.

see liquibase/liquibase-hanadb#69

Expected/Desired Behavior

A clear and concise description of what happens in the software after this pull request.

Screenshots (if appropriate)

If applicable, add screenshots to help explain your problem.

Additional Context

Add any other context about the problem here.

Fast Track PR Acceptance Checklist:

Need Help?

Come chat with us on our discord channel

This is useful in case an extension has to override
liquibase.snapshot.jvm.UniqueConstraintSnapshotGenerator#listConstraints.
@breglerj
Copy link
Contributor Author

@r2liquibase can you please take a look at this? It will help fixing liquibase/liquibase-hanadb#69.

@molivasdat
Copy link
Contributor

HI @breglerj Thanks for creating this issue.

A member of the Liquibase team will take a look at your contribution and may suggest:

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

@molivasdat molivasdat added ImpactLow IntegrationCLI RiskMedium Changes that require more testing or that affect many different code paths. Severity4 TypeBug labels Sep 16, 2021
@molivasdat
Copy link
Contributor

Part of this fix may belong in the hanadb extension.

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.

Code review and test results:

I've been hoping to make some significant changes to this API for a while, and so the methods were not extensible because I wasn't sure how compatible we'll continue to be long-term as we do things like support nosql databases etc.

But, that is likely a while from now still that we'll get to that and we will already have to deal with compatibility issues at this point, so no use blocking people from extending things as they need now.

Things to be aware of:

  • Doesn't change behavior, just makes methods extendable

Things to worry about:

  • Nothing

@nvoxland nvoxland added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Aug 29, 2022
@github-actions
Copy link

Unit Test Results

  4 620 files  ±0    4 620 suites  ±0   34m 40s ⏱️ - 2m 30s
  4 617 tests ±0    4 402 ✔️ +4     215 💤  - 4  0 ±0 
54 576 runs  ±0  49 556 ✔️ +4  5 020 💤  - 4  0 ±0 

Results for commit 478d9f0. ± Comparison against base commit e742a88.

@nvoxland nvoxland changed the base branch from master to 1_9 August 29, 2022 23:13
@nvoxland nvoxland changed the base branch from 1_9 to master August 29, 2022 23:14
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 changes several snapshot classes to protected to allow extensibility.
  • Fix does not impact functionality of current implementations leveraging these classes.
  • No additional testing required.

APPROVED

@nvoxland nvoxland merged commit 90f84a2 into liquibase:master Aug 31, 2022
@kataggart kataggart added this to the NEXT milestone Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autocandidate complexityLocal criticalityBlocker DBHana enabler Extensions ImpactLow IntegrationCLI RiskMedium Changes that require more testing or that affect many different code paths. SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions Severity3 Severity4 sprint2022-32 TypeBug ver4.5.0
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants