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

Refactor start-h2 logic #2597

Merged
merged 11 commits into from
May 12, 2022
Merged

Refactor start-h2 logic #2597

merged 11 commits into from
May 12, 2022

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Mar 3, 2022

Description

Moved StartH2Main from being a stand-alone command to being integrated into the StartH2CommandStep.

The old examples/start-h2 scripts are still there, but are now calls to liquibase init start-h2 rather than a separately boostrapped java call to StartH2Main.

Fixes #2481

…dStep and the old start-h2 standalone script is just a call to `liquibase init start-h2`
…dStep and the old start-h2 standalone script is just a call to `liquibase init start-h2`
@nvoxland
Copy link
Contributor Author

nvoxland commented Mar 3, 2022

Code review and test results:

Things to be aware of

  • There were two copies of the start-h2 scripts, I deleted the uneeded copy and made sure the remaining copy was correctly copied (preserved unix permissions and dos/unix line endings ) into the final tar.gz file
    • Would like a double-check that these files still work from official build
  • The code in StartH2CommandStep is copy-pasted from the old StartH2Main
    • But I did move some hard-coded settings like ports and username/password to be arguments now that that is easy to do
  • I found the h2.bindAddress system property, which looks like the one h2 uses to decide the IP/host to connect to. Without that setting, it would guess at the best one and sometimes get it wrong.
    • In our case, we want it to be localhost for security reasons, but there is a new arg for people who don't want that
  • There is a corresponding pro PR and also Remove references to examples/start-h2 scripts liquibase-docs#137

Things to worry about

  • Does the old start-h2 scripts still work on mac/linux and windows? Does liquibase init start-h2 still work?
    • The "does it start" is the main thing that changed. The logic within is mostly the same.

@nvoxland nvoxland requested a review from suryaaki2 March 3, 2022 16:41
@nvoxland nvoxland added this to To Do in Conditioning++ via automation Mar 3, 2022
@StevenMassaro
Copy link
Contributor

@nvoxland Are you going to remove the com.datical.liquibase.ext.command.init.StartH2CommandStep from the pro code?

@nvoxland
Copy link
Contributor Author

@nvoxland Are you going to remove the com.datical.liquibase.ext.command.init.StartH2CommandStep from the pro code?

Yes, there is a corresponding PR that removes that

@github-actions
Copy link

github-actions bot commented Mar 10, 2022

Unit Test Results

  4 512 files  ±  0    4 512 suites  ±0   36m 32s ⏱️ + 3m 56s
  4 389 tests +  3    4 175 ✔️ +  3     214 💤 ±0  0 ±0 
51 948 runs  +36  46 940 ✔️ +36  5 008 💤 ±0  0 ±0 

Results for commit 73de25c. ± Comparison against base commit ba79dd1.

♻️ This comment has been updated with latest results.

# Conflicts:
#	liquibase-core/src/main/resources/liquibase/examples/start-h2
#	liquibase-core/src/main/resources/liquibase/examples/start-h2.bat
#	liquibase-dist/src/main/archive/examples/start-h2
#	liquibase-dist/src/main/archive/examples/start-h2.bat
@kataggart kataggart added the DBH2 label May 2, 2022
@StevenMassaro
Copy link
Contributor

@nvoxland I've noticed that when I CTRL+C in the prompt where H2 is running, it takes a few seconds before the command is interrupted. Is that something that can be fixed, and is it fixed by your changes here?

@nvoxland
Copy link
Contributor Author

nvoxland commented May 6, 2022

I added a commit to better handle that pause after "ctrl-c"

I also updated the readme and getting_started pages

@FBurguer
Copy link

FBurguer commented May 11, 2022

This fix adds a new command liquibase init start-h2 to start an example h2 DB, keeping the bat script (the bat command uses liquibase init start-h2 now too). The only functional test that i could think so are using liquibase init start-h2 command with this build and then use the bat file on an older build and compare their behavior.
Both ways work the same way so i procided to check bugfixes and the things to be aware of by Nathan:

  1. Retain start-h2 files in the examples directory, but they call liquibase init start-h2 and not StartH2Main. PASS
  2. No documentation references to start-h2, only liquibase init start-h2 PASS
  3. If you run liquibase init start-h2 with no h2 jar file in your liquibase/lib dir, the error message is valid PASS
  4. Browser opens to "localhost" not an IP address FAIL
  5. URLs shown in the console output are "localhost" and not an IP address FAIL
    (4 and 5 failed in my environment but after trying on my coworker environement it looks like my case is uncommon, it is a thing to be aware of though)

Test environment
Liquibase Version: [Core: //start-h2-simplify/2477/50379f/2022-05-02 17:04+0000, Pro: start-h2-simplify/1078/c6d556/2022-05-02T16:59:19Z] #2477 built at 2022-05-02 17:04+0000
h2 2
jdk 1.8.0_291
Windows 10 Enterprise

@kataggart kataggart added this to the NEXT milestone May 12, 2022
@nvoxland nvoxland merged commit 793c760 into master May 12, 2022
Conditioning++ automation moved this from To Do to Done May 12, 2022
@filipelautert filipelautert deleted the start-h2-simplify branch April 25, 2023 12:09
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.

Simplify start-h2 usage and code
5 participants