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

Fetch ran change sets before updating check sums #4762

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Fetch ran change sets before updating check sums #4762

merged 1 commit into from
Sep 19, 2023

Conversation

hpoettker
Copy link
Contributor

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

Fixes #4460.

The fix ensures that the field storedFilePath for each changeSet in UpdateChangeSetChecksumGenerator is set. Then, the WHERE condition of the produced UPDATE statements matches the database entry for the respective change set.

Things to be aware of

Things to worry about

Additional Context

@filipelautert filipelautert added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Aug 28, 2023
@filipelautert filipelautert self-assigned this Aug 28, 2023
@filipelautert filipelautert added TypeBug 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 Aug 28, 2023
@filipelautert
Copy link
Collaborator

@hpoettker interesting, this change is causing some tests to fail. I'm trying to figure out why.. any ideas?

@hpoettker
Copy link
Contributor Author

@filipelautert Do you mean the integration tests for Oracle and MariaDB?

That is indeed curious. Both tests are failing when creating the sequence seqtestdatatypeonall with

  • Oracle: CREATE SEQUENCE LBUSER.seqtestdatatypeonall AS int
  • MariaDB: CREATE SEQUENCE lbcat.seqtestdatatypeonall AS int

I'd say that the AS int might work on PostgreSQL but is indeed invalid syntax for Oracle or MariaDB.

I don't see how my proposed change could cause such a problem. I also don't see how I can easily reproduce the test setup for the integration tests.

@filipelautert filipelautert 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 Aug 29, 2023
@hpoettker
Copy link
Contributor Author

I found the yaml to get the integration tests working locally. 😄

The original PR was indeed erroneous. Using the constructor of ChangeLogIterator that takes the list of ran change sets seemed like an elegant idea at first sight. But it really does something much more specific than setting the stored file path in change sets.

I've also amended UpgradeChecksumVersionIntegrationTest with a test to reproduce the issue that this PR addresses.

@filipelautert filipelautert added sprint2023-58 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 Aug 30, 2023
Copy link
Collaborator

@filipelautert filipelautert left a comment

Choose a reason for hiding this comment

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

Nice, good idea to fix it on ValidatingVisitor. In fact the Visitor is becoming a "fix it all" Visitor - something to think about .

@rberezen functional tests are passing here -> https://github.com/liquibase/liquibase-pro-tests/actions/runs/6038104842

@filipelautert filipelautert added this to the 1NEXT milestone Sep 19, 2023
@filipelautert filipelautert merged commit 72a0c2d into liquibase:master Sep 19, 2023
10 of 13 checks passed
@hpoettker hpoettker deleted the fetch-ran-change-sets branch September 19, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint2023-58 TypeBug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Checksums are not actually upgraded when FILENAME starts with classpath:
4 participants