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

minor bash fixes in start-h2 & user experience requests #1241

Closed
wants to merge 1 commit into from

Conversation

metasean
Copy link

@metasean metasean commented Jun 25, 2020


name: Pull Request
about: Bug fixes and User Experience
title: 'Fix some low hanging bugs; Suggest other more in-depth fixes'
labels: Status:Discovery
assignees: ''


Environment

Liquibase Version: 3.10.0

Liquibase Integration & Version: CLI

Liquibase Extension(s) & Version: n/a

Database Vendor & Version: n/a

Operating System Type & Version: macOS Catalina v10.15.4

Pull Request Type

  • Bug fix (non-breaking change which fixes an issue.)
  • Enhancement/New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

I'm brand new to Liquibase and trying to start up the examples shell on macOS Catalina.

User Experience Problem #1

My initial problem was when I called the start-h2 script, I got the following:

~ % /usr/local/opt/liquibase/examples/start-h2
/usr/local/opt/liquibase/examples/start-h2: line 8: [: missing `]'
/usr/local/opt/liquibase/examples/start-h2: line 16: [: missing `]'
Starting Example H2 Database...
NOTE: The database does not persist data, so stopping and restarting this process will reset it back to a blank database

Error starting H2
java.lang.reflect.InvocationTargetException
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:564)
        at liquibase.example.StartH2Main.startWebServer(StartH2Main.java:75)
        at liquibase.example.StartH2Main.main(StartH2Main.java:29)
Caused by: org.h2.jdbc.JdbcSQLNonTransientConnectionException: Exception opening port "8090" (port may be in use), cause: "java.net.BindException: Address already in use" [90061-200]
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:622)
        at org.h2.message.DbException.getJdbcSQLException(DbException.java:429)
        at org.h2.message.DbException.get(DbException.java:194)
        at org.h2.util.NetUtils.createServerSocketTry(NetUtils.java:180)
        at org.h2.util.NetUtils.createServerSocket(NetUtils.java:146)
        at org.h2.server.web.WebServer.start(WebServer.java:389)
        at org.h2.tools.Server.start(Server.java:511)
        ... 6 more
Caused by: java.net.BindException: Address already in use
        at java.base/sun.nio.ch.Net.bind0(Native Method)
        at java.base/sun.nio.ch.Net.bind(Net.java:479)
        at java.base/sun.nio.ch.Net.bind(Net.java:468)
        at java.base/sun.nio.ch.NioSocketImpl.bind(NioSocketImpl.java:643)
        at java.base/java.net.ServerSocket.bind(ServerSocket.java:396)
        at java.base/java.net.ServerSocket.<init>(ServerSocket.java:282)
        at java.base/java.net.ServerSocket.<init>(ServerSocket.java:173)
        at org.h2.util.NetUtils.createServerSocketTry(NetUtils.java:176)
        ... 9 more

The first problem I tackled was the Exception opening port "8090" (port may be in use).

It turns out that this is the port used by Docker Desktop for Macs, which was running in the background.

Some type of port check and clearer notification really would have been appreciated here!

Part of a possible solution

Something like the following at the beginning of the start-h2 file will work for macOS users, but you should obviously have a more platform-agnostic approach, hence no PR for this portion.

PORT="8090" # I assume you can pass this in from where ever it is actually set

# the PORT_IN_USE check works on Mac, and presumably other POSIX systems,
# but it probably won't work on Windows machines
PORT_IN_USE=$(lsof -i TCP:$PORT | grep LISTEN | awk '{print $2}')
if [ $PORT_IN_USE ]; then
  echo "Must stop process running on port $PORT"
  exit 1
fi

Bug #1

Once I had closed my Docker Desktop, I reran the sh start-h2 script and I got this:

examples % sh start-h2                                  
start-h2: line 8: [: missing `]'
start-h2: line 16: [: missing `]'
Starting Example H2 Database...
NOTE: The database does not persist data, so stopping and restarting this process will reset it back to a blank database

Connection Information:
  Dev database: 
    JDBC URL: jdbc:h2:tcp://localhost:9090/mem:dev
    Username: dbuser
    Password: letmein
  Integration database: 
    JDBC URL: jdbc:h2:tcp://localhost:9090/mem:integration
    Username: dbuser
    Password: letmein

Opening Database Console in Browser...
  Dev Web URL: http://localhost:8090/frame.jsp?jsessionid=dddfc09b9992bd48221f72c3f3b8b33d
  Integration Web URL: http://localhost:8090/frame.jsp?jsessionid=800d161e8249f7eccdf6af62ba7ae1d3

The lines in question actually have an error in the formatting of the if statements.

# Instead of:   
                if [ -z "${LIQUIBASE_PATH}"]; then
# it should be:   
                if [ -z "${LIQUIBASE_PATH}" ]; then
# Likewise,
# instead of:   
                if [ -z "${JAVA_HOME}"]; then
# it should be:    
                if [ -z "${JAVA_HOME}" ]; then

This PR should fixes these.


User Experience Problem #2

Frankly, the Tutorial for Beginners: Using the Liquibase Installer page is particularly not useful.

For example, nowhere does it indicate that sh start-h2 should be run.

I also still have no idea what "To get started, download your database’s JDBC driver to the lib directory, then run the Liquibase script in the root of this directory." means or where to look for JDBC drivers.


Fast Track PR Acceptance Checklist:

Need Help?

Come chat with us on our discord channel

@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #1241 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1241   +/-   ##
=========================================
  Coverage     47.72%   47.72%           
  Complexity     7474     7474           
=========================================
  Files           757      757           
  Lines         36276    36276           
  Branches       6627     6627           
=========================================
+ Hits          17312    17313    +1     
  Misses        16660    16660           
+ Partials       2304     2303    -1     
Impacted Files Coverage Δ Complexity Δ
...e/src/main/java/liquibase/util/DependencyUtil.java 92.98% <0.00%> (+0.87%) 0.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17fcfe4...08cd834. Read the comment docs.

@mariochampion
Copy link
Contributor

hey @metasean -- thanks for the specific issues and suggested fix for Bug #1. I have seen that one so long, and because it just starts up, I have just ignored it -- but we should correct for it. And we'll get into the user experience frictions as well. thanks much for spending the time to get into the details!

@mariochampion
Copy link
Contributor

hey @nvoxland -- i just made metasean's tweaks to a local test copy and they worked for me.
Screen Shot 2020-06-25 at 4 12 35 PM, if you want to count that as a review for approving this PR.

(note: there are separate tickets to be made for the other ux issues.)

@filipelautert
Copy link
Collaborator

This has been fixed by PR ##2263 2 years ago. Anyway thanks for this one !

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

Successfully merging this pull request may close these issues.

None yet

7 participants