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

Remove snowflake unsupported logging from snapshot command (DAT-11291) #3277

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

abrackx
Copy link
Contributor

@abrackx abrackx commented Sep 16, 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

Removes message INFO This command might not yet capture Liquibase Pro additional object types on snowflake when running snapshot against snowflake.

Things to be aware of

Removed duplicate code from SnapshotCommandStep.

Things to worry about

N/A

Additional Context

N/A

@github-actions
Copy link

github-actions bot commented Sep 16, 2022

Unit Test Results

  4 644 files  ±0    4 644 suites  ±0   36m 37s ⏱️ + 5m 20s
  4 626 tests ±0    4 411 ✔️ ±0     215 💤 ±0  0 ±0 
54 684 runs  ±0  49 664 ✔️ ±0  5 020 💤 ±0  0 ±0 

Results for commit 987abb5. ± Comparison against base commit 6c6bb0f.

♻️ This comment has been updated with latest results.

@abrackx abrackx marked this pull request as ready for review September 16, 2022 13:56

public static void logUnsupportedDatabase(Database database, Class callingClass) {
if (LicenseServiceUtils.isProLicenseValid()) {
if (!(database instanceof MSSQLDatabase
|| database instanceof OracleDatabase
|| database instanceof MySQLDatabase
|| database instanceof DB2Database
|| database instanceof PostgresDatabase)) {
|| database instanceof PostgresDatabase
|| database.getShortName().equalsIgnoreCase("snowflake"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to do this hacky because adding a dependency on the snowflake module would cause a circular dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Until that module is fully integrated into core we've got to use this hack. I talked it through with @nvoxland but I should probably also add a TODO. I'll do that now!

Copy link
Contributor

Choose a reason for hiding this comment

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

🤮

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.

It is the easiest way to make the change. We'd like something different, but it will take some thought on how to organize the code

@suryaaki2 suryaaki2 merged commit 165c594 into master Sep 21, 2022
@suryaaki2 suryaaki2 deleted the DAT-11291 branch September 21, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants