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

Use path instead of absolutePath for resources #2122

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

liefke
Copy link
Contributor

@liefke liefke commented Sep 28, 2021

Environment

Liquibase Version: 4.5.0
Liquibase Integration & Version: CLI

Pull Request Type

[x] Bug fix (fixes #2121)

Description

When trying to read properties files from the class path, file.getAbsolutePath() is used. The result is some absolute path which won't exist in the classpath on most systems.

Steps To Reproduce

See the description in issue #2121


Dev Handoff Notes (Internal Use)

Links

Testing

  • Steps to Reproduce: The steps in liquibase.properties not read from classpath #2121 only worked when calling the old liquibase.integration.commandline.Main class directly and not using the new LiquibaseCommandLine class. So to reproduce you'll have to tweak the liquibase.bat script to use the old classname instead.
  • Guidance:
    • Impact: Does not impact people using the new LiquibaseCommandLine class since the validation is done through different code now. The changed code is still called, even in the new code paths, but any values found are not used

Dev Verification

Verified the issue when using Main directly and that the fix addresses it.
Verified the fix does not cause issues when using LiquibaseCommandLine and that the reported issue is still working with the fix

┆Issue is synchronized with this Jira Bug by Unito

@molivasdat
Copy link
Contributor

Thanks @liefke for adding this fix.
A member of the Liquibase team will take a look at your contribution and may suggest:

The PR will be prioritized according to our internal development and testing capacity.

We’ll let you know when it’s ready to move to the next step or if any changes are needed.

@kataggart kataggart added this to To Do in Conditioning++ via automation Dec 27, 2021
@kataggart kataggart moved this from To Do to In discussion in Conditioning++ Dec 27, 2021
@kataggart kataggart moved this from In discussion to To Do in Conditioning++ Jan 4, 2022
@nvoxland
Copy link
Contributor

nvoxland commented Jan 5, 2022

This change is not strictly needed anymore because the logic of handling the properties files now mainly exists in the LiquibaseCommandLine class which is replacing Main. Given that you are seeing the bug, I assume you're calling Main directly, @liefke vs. using the liquibase.bat/liquibase script? I wasn't able to replicate the issue unless I was calling Main directly.

But, the change does help people that are still calling Main directly, so it's worth bringing in still. Thanks!

@nvoxland nvoxland moved this from To Do to Code Review in Conditioning++ Jan 5, 2022
@nvoxland nvoxland changed the base branch from master to 1_9 January 5, 2022 21:16
@nvoxland nvoxland changed the base branch from 1_9 to master January 5, 2022 21:16
@liefke
Copy link
Contributor Author

liefke commented Jan 5, 2022

@nvoxland I have to admit that I'm still using a version wich has no LiquibaseCommandLine. But as the mentioned code still exists in master, I was thinking that it is better to fix it anyway.

@suryaaki2 suryaaki2 moved this from Code Review to Ready for Handoff (In JIRA) in Conditioning++ Jan 6, 2022
@sync-by-unito sync-by-unito bot assigned nvoxland and yodzhubeiskyi and unassigned nvoxland Jan 10, 2022
@XDelphiGrl
Copy link
Contributor

The fix allows use of the pre-4.1.0 Liquibase main class to execute CLI commands that depend on a jar containing Liquibase configuration files. If the jar including the Liquibase configuration files is in the Liquibase classpath, you no longer get "missing required parameter" errors for properties that are correctly defined in a liquibase.properties file in the configuration jar. Note that by calling the the older version of main, you need to use the legacy-version of command and parameter names; for example, you need to use updateSQL instead of update-sql and changeLogFile instead of liquibase.command.changelogFile.

Pre-4.1.0 Main Class: liquibase.integration.commandline.Main
Current Main Class: liquibase.integration.commandline.LiquibaseCommandLine


Notes on Setup
  • Create a liquibase.properties file with required properties to connect to a database: url, username and password.
    • Include the changeLogFile property, too, despite it not being required.
  • Put the liquibase.properties file, the changelog file and the JDBC driver for your database in one directory.
  • CD to the directory and package a jar for your liquibase configuration files.
    jar cvf liquibase-config.jar liquibase.properties changelog.xml driver.jar
  • Move liquibase-config.jar into the liquibase/lib directory.

Liquibase CLI Command Syntax

java -cp "liquibase.jar;lib/*;liquibase-config.jar" liquibase.integration.commandline.Main updateSQL


  • Verify updateSQL uses configuration files located in the liquibase-config.jar. PASS

    • There is no error about missing --url option PASS
    • There is no error about missing --changeLogFile option PASS
    • SQL is generated for the undeployed changesets in the changelog PASS
  • Verify update uses configuration files located in the liquibase-config.jar. PASS

    • There is no error about missing --url option PASS
    • There is no error about missing --changeLogFile option PASS
    • Objects are created in the database PASS

Test Environment
Liquibase Core: issue-2121/1123/89513b/2022-01-05 21:13+0000, Pro: master/367/8a810b/
Postgres 12
Windows 10
Passing Functional Tests

@nvoxland nvoxland merged commit fe8fadf into liquibase:master Feb 14, 2022
Conditioning++ automation moved this from Ready for Handoff (In JIRA) to Done Feb 14, 2022
@nvoxland nvoxland added this to the v4.8.0 milestone Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

liquibase.properties not read from classpath
6 participants