Skip to content

update-to-tag fixes when provided tag doesn't exist or there is not tagDatabaseChange in the changelog #6169

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Aug 30, 2024
Merged

update-to-tag fixes when provided tag doesn't exist or there is not tagDatabaseChange in the changelog #6169

merged 13 commits into from
Aug 30, 2024

Conversation

MalloD12
Copy link
Collaborator

@MalloD12 MalloD12 commented Aug 6, 2024 •
edited
Loading

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 #6126

When running liquibase update-to-tag --tag=myTag, two different non-ideal things can happen:

  1. If there is no tagDatabase change type in the changelog file, the entire changelog will run anyway.
  2. If the provided tag doesn't exist (for example if it's misspelled), all changes will run.

To keep backward compatibility with some people's usage we have not made any specific change to the current behavior, but what we have done on this PR is to introduce the usage of STRICT global configuration for the two above cases. Now, if STRICT=true in the first case a TagDatabaseChange not found error message will be displayed and in the second case a message letting the user know there is no matching between the command tag argument and the tag specified in the change.

Things to be aware of

Things to worry about

Additional Context

@MalloD12 MalloD12 self-assigned this Aug 6, 2024
@MalloD12 MalloD12 requested a review from filipelautert as a code owner August 6, 2024 14:12
@MalloD12 MalloD12 requested a review from tati-qalified August 6, 2024 14:45
@MalloD12 MalloD12 changed the title Fix issue 6126 update-to-tag fixes when provided tag doesn't exist or there is not tagDatabaseChange in the changelog Aug 6, 2024
@MalloD12 MalloD12 requested a review from filipelautert August 7, 2024 22:25
@MalloD12 MalloD12 requested a review from filipelautert August 20, 2024 16:42
filipelautert
filipelautert requested changes Aug 20, 2024
View reviewed changes
liquibase-standard/src/main/java/liquibase/command/core/UpdateToTagCommandStep.java Outdated

if(GlobalConfiguration.STRICT.getCurrentValue()) {
TagDatabaseChange tagDatabaseChange = getTagDatabaseChange(changeLog.getChangeSets());
boolean thereIsTagDatabaseChange = tagDatabaseChange != null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems the boolean value found at line 107 is doing the same logic as getTagDatabaseChange : found is going through all the changelogs and then getTagDatabaseChange is doing the same thing - just one use streams and the other one for loops.
Both codes could be merged so you don't need to iterate over changelogs twice. For instance: found could hold a TagDatabaseChange instead of a boolean and then you can use it for the STRICT logic here too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I saw it but decide to go with this change as it is, because found boolean flag considers both cases together (tagDatabaseChange exits and the runtime tag matches with the change's tag).

@MalloD12 MalloD12 requested a review from filipelautert August 21, 2024 16:24
@filipelautert filipelautert requested a review from rberezen August 27, 2024 16:50
@rberezen
Copy link
Contributor

this is a useful change, but want to notify other stakeholders about this implementation
one thing I'm concerned about is that we put too much on the --strict flag
there might be cases when user might want to use i.e. endDelimiter with --strict=true and update-to-tag --strict=false that will conflict each other
cc @suryaaki @lbadam @mariochampion

@rberezen
Copy link
Contributor

ran extra tests https://github.com/liquibase/liquibase-pro-tests/actions/runs/10620473636

@murrayadam
Copy link
Contributor

@rberezen that's a good point. I do agree that we need to be careful how we continue to add to strict. I would like to see us implement a system where we have a generic strict for users while also allowing them to toggle individual properties like https://www.typescriptlang.org/tsconfig/#strict

@filipelautert filipelautert added this to the 1NEXT milestone Aug 30, 2024
@filipelautert filipelautert merged commit da5dfed into master Aug 30, 2024
44 checks passed
@filipelautert filipelautert deleted the fix-issue-6126 branch August 30, 2024 11:13
@MalloD12
Copy link
Collaborator Author

I agree with @rberezen, I think at some point there would be some feature crashing using strict flag. In the past, we had some related discussions where we were discussing whether to overload strict or have separate global configurations that allow us to control some specific behavior, but we decided to keep using strict for this purpose instead of increase more and more this list (of configurations) up to a point it gets unmaintainable. I think something similar is happening now with strict flag, I'm thinking maybe we can go in the direction of a middle point of having a few strict flags to control command-specific behaviors, or to control some other properties specific behaviors. For example:

--strict-update

--stric-changesetProperties (or some other names, but I'm pointing out here cases like endDelimiter or any other).

...

etc

What do you guys think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filipelautert filipelautert filipelautert approved these changes

@rberezen rberezen rberezen approved these changes

@tati-qalified tati-qalified tati-qalified approved these changes

Assignees

@MalloD12 MalloD12

Labels
DocNeeded TypeEnhancement
Projects
Liquibase Open Source
Status: Done
Milestone
v4.30.0
Development

Successfully merging this pull request may close these issues.

update-to-tag runs even when provided tag doesn't exist
5 participants
@MalloD12 @rberezen @murrayadam @filipelautert @tati-qalified