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.

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

Introduced "contextFilter" and "labelFilter" replacement settings #2971

Merged
merged 11 commits into from
Aug 18, 2022
Merged

Introduced "contextFilter" and "labelFilter" replacement settings #2971

merged 11 commits into from
Aug 18, 2022

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Jun 17, 2022 •
edited

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

Contexts vs. Labels is a common confusion. I think part of the problem is that the names of the attributes are just "labels" and "context(s)" on both the runtime arguments and the changelog parameters so it's non-obvious how one side of each is a "filter" expression and which side is the filter is the difference.

This PR:

  • Switches all the changelog file use of "context" to "contextFilter"
    • changeSet attribute
    • include attribute
    • includeAll attribute
    • modifySql attribute
    • databaseChangeLog attribute
    • In XML, Yaml, JSON, and formatted SQL changelog formats
  • Switches the use of "labels" to "labelFilter" as an argument to:
    • CLI
    • Maven
    • Ant
    • Servlet
    • Spring
  • Preserves old context/labels functionality for backwards compatibility
    • Introduces "alias" support to CommandArgumentDefinition
    • Keeps old get/set methods, but marked as deprecated and calling the new get/set methods
  • Testing:
    • Updates unit and integration tests to use contextFilter
    • Adds additional unit and integration tests for context/labels compatibility

Things to be aware of

  • Lots of files got changed, but it was primarily through automated refactoring of the get/setLabels and get/setContexts functions and/or straightforward search/replace
    • Existing automated tests caught the couple places I missed. The functionality worked for me on the first try
  • Did not update any test-harness or functional tests changelogs, so they are currently testing the backwards compatibility
  • CLI --help just lists labelFilter, not labels anymore.
    • No need to remind people of the old and confusing argument

Things to worry about

  • Are there usages of contexts or labels I missed?

@nvoxland
- Deprecated the "context" attribute in changelog in favor of context…
ff28af5 
…Filter

- Deprecated the "labels" command arguments in favor of labelFilter
- Updated integration test changelogs to use dbchangelog-next instead of random old versions
@github-actions
Copy link

github-actions bot commented Jun 17, 2022 •
edited

Unit Test Results

  4 620 files    4 620 suites   34m 27s ⏱️
  4 612 tests   4 397 ✔️    215 💤 0 ❌
54 516 runs  49 496 ✔️ 5 020 💤 0 ❌

Results for commit 2bf9748.

♻️ This comment has been updated with latest results.

@nvoxland
- Deprecated the "context" attribute in changelog in favor of context…
210a052 
…Filter

- Deprecated the "labels" command arguments in favor of labelFilter
- Updated integration test changelogs to use dbchangelog-next instead of random old versions
@nvoxland
- Deprecated the "context" attribute in changelog in favor of context…
8280b7a 
…Filter

- Deprecated the "labels" command arguments in favor of labelFilter
- Updated integration test changelogs to use dbchangelog-next instead of random old versions
@nvoxland nvoxland added this to To Do in Conditioning++ via automation Jun 18, 2022
@nvoxland nvoxland changed the title Improved context and label arguments Introduced "contextFilter" and "labelFilter" replacement settings Jun 18, 2022
wwillard7800
wwillard7800 approved these changes Jun 20, 2022
View reviewed changes
Copy link
Contributor

@wwillard7800 wwillard7800 left a comment

Choose a reason for hiding this comment

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

Looks good

@kataggart kataggart self-assigned this Jun 20, 2022
@kataggart kataggart self-requested a review June 20, 2022 19:38
@kataggart
Copy link
Contributor

kataggart commented Jul 19, 2022 •
edited

@nvoxland I finally read this and completely understand! Does your change make this backwards compatible? So if I have changesets that still have context instead of 'contextFilter' they will still work? Similarly, if I use labels instead of 'labelFilter' in my CLI argument it will still work? I believe that is the case, but I wanted to confirm here. Thanks.

@kataggart
Copy link
Contributor

@nvoxland IGNORE ME! I can see this here now:

Preserves old context/labels functionality for backwards compatibility

Introduces "alias" support to CommandArgumentDefinition
Keeps old get/set methods, but marked as deprecated and calling the new get/set methods

I'd like to make sure we test that both the legacy and new alias values work as part of the testing here. Thanks.

kataggart
kataggart reviewed Jul 19, 2022
View reviewed changes
Copy link
Contributor

@kataggart kataggart left a comment

Choose a reason for hiding this comment

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

all good by me; I am just working to make sure folks on the other team don't have concerns

@kataggart kataggart assigned nvoxland and unassigned kataggart Jul 19, 2022
@kataggart kataggart requested review from kataggart and removed request for kataggart July 21, 2022 19:35
@nvoxland nvoxland requested a review from suryaaki2 July 26, 2022 21:40
@ecarneiro01 ecarneiro01 self-assigned this Jul 28, 2022
@nvoxland nvoxland removed their assignment Jul 28, 2022
@kataggart kataggart added this to the NEXT milestone Jul 28, 2022
@kataggart kataggart added the ReleaseMajor Fix needs to be part of an x.y release, not an x.y.z patch release label Aug 1, 2022
@kataggart kataggart removed this from the NEXT milestone Aug 1, 2022
@kataggart kataggart linked an issue Aug 1, 2022 that may be closed by this pull request
Wrong documentation: contextFilter is not a valid attribute #3052
Closed
@kataggart kataggart added this to the NEXT milestone Aug 4, 2022
@ecarneiro01
Copy link

A review of old and new naming of context and labels was done:

  • XML, YML, JSON changelogs:

    • Both "context" and "contextFilter" can be used as an attribute in the following tags:
      • changeSet PASS
      • include PASS
      • includeAll PASS
      • modifySql PASS
      • databaseChangeLog PASS

  • SQL changelogs:

    • Both "context" and "contextFilter" are supported for changesets in sql formatted changelogs.

  • "labels" and "labelFilter" are supported arguments in:

    • Servlet PASS
    • Ant PASS
    • CLI PASS
    • Maven PASS

The new argument "labelFilter" can't be used in spring at the moment given we don't manage that integration.

@nvoxland nvoxland merged commit 19a15f5 into master Aug 18, 2022
Conditioning++ automation moved this from To Do to Done Aug 18, 2022
@nvoxland nvoxland deleted the context-and-label-filter-names branch August 18, 2022 21:48
@filipelautert filipelautert mentioned this pull request Dec 2, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kataggart kataggart kataggart left review comments

@wwillard7800 wwillard7800 wwillard7800 approved these changes

@suryaaki2 suryaaki2 suryaaki2 approved these changes

Assignees

@ecarneiro01 ecarneiro01

Labels
DocNeeded featureContext featureLabels marketingRequired ReleaseMajor Fix needs to be part of an x.y release, not an x.y.z patch release sprint2022-31 TypeEnhancement
Projects
Liquibase Open Source
Archived in project
Conditioning++
Done
Milestone
v4.16.0
Development

Successfully merging this pull request may close these issues.

Wrong documentation: contextFilter is not a valid attribute
5 participants
@nvoxland @kataggart @ecarneiro01 @wwillard7800 @suryaaki2