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

Fix generateChangeLog and diffChangeLog logic to avoid including default schema name when it should not #3246

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

MalloD12
Copy link
Contributor

@MalloD12 MalloD12 commented Sep 7, 2022

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

Fix generateChangeLog and DiffChangeLog logic to avoid including default schema name when includeSchema=false.

FIXES #2776

Things to be aware of

  • Update logic by setting up outputDefaultSchema flag depending on value set of includeSchema flag parameter.
  • Only impacts generateChangeLog and diffChangelog CLI.

Things to worry about

  • Nothing

Additional Context

  • Nothing

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

Unit Test Results

  4 656 files  ±0    4 656 suites  ±0   39m 38s ⏱️ - 4m 11s
  4 628 tests ±0    4 413 ✔️ ±0     215 💤 ±0  0 ±0 
54 708 runs  ±0  49 688 ✔️ ±0  5 020 💤 ±0  0 ±0 

Results for commit a8933f4. ± Comparison against base commit 031928b.

♻️ This comment has been updated with latest results.

…chema name when includeSchame flag is set to false
@MalloD12 MalloD12 assigned nvoxland and unassigned MalloD12 Sep 26, 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.

Code review and test results:

Things to be aware of:

  • The changes make sense to me
  • New tests (and changed behavior of existing setup) is shown in liquibase/liquibase-pro-tests#447 and those look correct
  • Only impacts generateChangeLog with formatted sql
    The generated sql will be different for people that are used to having the schema included. But they can use the existing flags to (now correctly) configure that behavior. It's good the formatted sql output now matches the default values.

Things to worry about:

  • Nothing

@nvoxland nvoxland removed their assignment Sep 26, 2022
@XDelphiGrl
Copy link
Contributor

@MalloD12 and @nvoxland, hi -

In reading the linked issue, it looks like the problematic command included both include-schema and schemas.

  • Does the bug occur only when the schemas attribute was used in conjunction with include-schema false?
  • Does the bug reproduce if include-schema is omitted from the problematic command? I ask because the default for include-schema is false.
  • If the answer to either of those questions is "yes", the functional tests will need to be adjusted.

Thanks!

@MalloD12
Copy link
Contributor Author

@MalloD12 and @nvoxland, hi -

In reading the linked issue, it looks like the problematic command included both include-schema and schemas.

  • Does the bug occur only when the schemas attribute was used in conjunction with include-schema false?
  • Does the bug reproduce if include-schema is omitted from the problematic command? I ask because the default for include-schema is false.
  • If the answer to either of those questions is "yes", the functional tests will need to be adjusted.

Thanks!

Hi @XDelphiGrl, sorry for the late reply but I just saw these questions. Below my answers:

  1. The problem here was basically how include-schemas, because even without specifying any schema but saying to liquibase to do not include schema (include-schema=false) when generating an sql format changelog default schema name was present. So that's what we have fixed.
  2. Yeap, behaviour is the same. If we don't specify include-schema=false default schema name is also present when generating changelog.

Thanks for your valuable questions, and again my apologises for my late response.

Daniel.

@XDelphiGrl
Copy link
Contributor

@MalloD12 and @nvoxland, hi -
In reading the linked issue, it looks like the problematic command included both include-schema and schemas.

  • Does the bug occur only when the schemas attribute was used in conjunction with include-schema false?
  • Does the bug reproduce if include-schema is omitted from the problematic command? I ask because the default for include-schema is false.
  • If the answer to either of those questions is "yes", the functional tests will need to be adjusted.

Thanks!

Hi @XDelphiGrl, sorry for the late reply but I just saw these questions. Below my answers:

1. The problem here was basically how include-schemas, because even without specifying any schema but saying to liquibase to do not include schema (`include-schema=false`) when generating an sql format changelog default schema name was present. So that's what we have fixed.

2. Yeap, behaviour is the same. If we don't specify `include-schema=false` default schema name is also present when generating changelog.

Thanks for your valuable questions, and again my apologises for my late response.

Daniel.

@MalloD12, no need to apologize! We are all very busy. Thank you for answering my questions, I appreciate the extra clarity.

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 prevents schema names from being included in generated changelogs when include-schema is false.

  • Functional tests were updated to match the output from the fixed Core code.
  • No additional testing required.

APPROVED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Postgres generateChangeLog includeSchema
6 participants