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

Support system properties as a way of providing configuration options to the CLI #4104

Conversation

jccampanero
Copy link
Contributor

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

The LiquibaseLauncher code, and the Liquibase CLI in general, have been enriched to support passing the different configuration options as JVM system properties.

Previously, such information could only be passed as operating system environment variables.

Fixes and augments, in order to provide a uniform way of reading the different configuration options, the scope of #3777.

Things to be aware of

Nothing.

Things to worry about

Nothing.

Additional Context

No test have been created for the issue.

@nvoxland
Copy link
Contributor

nvoxland commented May 3, 2023

Initial/Pre-Review Thoughts

The changes look good to me. Thanks!

Questions I have:

  • None

Potential risks:

  • None

What could make the full review difficult:

  • Nothing

@jccampanero
Copy link
Contributor Author

You are welcome @nvoxland Nathan.

@kevin-atx kevin-atx added TypeEnhancement RiskLow Trivial changes in spelling, documentation changes, focused bug fixes, etc. labels May 8, 2023
@filipelautert filipelautert self-assigned this May 17, 2023
@filipelautert filipelautert added sprint2023-51 SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels May 17, 2023
@filipelautert
Copy link
Collaborator

Hello @jccampanero ,maybe instead of using a Map from string to string, could not we use an Enum class where they key is similar to the string name (LIQUIBASE_HOME_JVM_PROPERTY_NAME ) and it would have a "value" field that would be the parameter value ("liquibase.home") . What do you think? This way we don't need to populate maps, etc.

@jccampanero
Copy link
Contributor Author

Hello @jccampanero ,maybe instead of using a Map from string to string, could not we use an Enum class where they key is similar to the string name (LIQUIBASE_HOME_JVM_PROPERTY_NAME ) and it would have a "value" field that would be the parameter value ("liquibase.home") . What do you think? This way we don't need to populate maps, etc.

Sorry for the late reply @filipelautert.

Yes, of course, I updated the code as requested. I used jvmPropertyName instead of value in the enum definition because I think it is more precise but please, feel free to adapt the code if you wish.

@filipelautert filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels May 19, 2023
@filipelautert
Copy link
Collaborator

Yes, of course, I updated the code as requested. I used jvmPropertyName instead of value in the enum definition because I think it is more precise but please, feel free to adapt the code if you wish.

Nice, I believe it is better now as we can see that those values are used only for this purpose, thanks for applying the changes!

@jccampanero
Copy link
Contributor Author

You are welcome @filipelautert, my pleasure.

@filipelautert filipelautert requested review from rberezen and removed request for XDelphiGrl June 28, 2023 13:20
@sayaliM0412 sayaliM0412 added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Aug 30, 2023
@sayaliM0412 sayaliM0412 temporarily deployed to external August 30, 2023 03:32 — with GitHub Actions Inactive
@sayaliM0412 sayaliM0412 temporarily deployed to external August 30, 2023 13:53 — with GitHub Actions Inactive
@jccampanero
Copy link
Contributor Author

Hello @jccampanero, I am testing some internal build process and using your branch as a guinea pig. Please let me know if you face any issues during this process. Thank you.

cc: @filipelautert

It is perfectly fine @sayaliM0412, please, feel free to use the branch for your tests, as you consider it appropriate.

@sayaliM0412 sayaliM0412 temporarily deployed to external August 31, 2023 21:18 — with GitHub Actions Inactive
@sayaliM0412 sayaliM0412 added the runFunctionalTests Trigger Pro-tests Functional Tests on a PR label Aug 31, 2023
@sayaliM0412 sayaliM0412 temporarily deployed to external August 31, 2023 21:19 — with GitHub Actions Inactive
@sayaliM0412 sayaliM0412 temporarily deployed to external August 31, 2023 21:21 — with GitHub Actions Inactive
@sayaliM0412 sayaliM0412 changed the base branch from github-action-DAT-15775 to master September 1, 2023 17:01
filipe added 3 commits October 12, 2023 10:10
…pport_for_jvm_properties

# Conflicts:
#	.github/workflows/build-branch.yml
#	.github/workflows/build.yml
#	liquibase-cli/src/main/java/liquibase/integration/commandline/LiquibaseLauncher.java
@filipelautert filipelautert added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Oct 12, 2023
@sonarcloud
Copy link

sonarcloud bot commented Oct 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@filipelautert filipelautert merged commit 9790070 into liquibase:master Oct 12, 2023
38 of 42 checks passed
@filipelautert filipelautert added this to the 1NEXT milestone Oct 12, 2023
@jccampanero jccampanero deleted the 3777_cli_provide_support_for_jvm_properties branch October 12, 2023 17:54
@adrian-velonis1
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DocNeeded RiskLow Trivial changes in spelling, documentation changes, focused bug fixes, etc. runFunctionalTests Trigger Pro-tests Functional Tests on a PR SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint2023-51 TypeEnhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants