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

fix(docker): update path to CLI jar #1241

Merged

Conversation

themightychris
Copy link
Collaborator

The current :latest Docker container build of the master branch is broken due to changes in #1229:

$ docker pull ghcr.io/mobilitydata/gtfs-validator:latest
latest: Pulling from mobilitydata/gtfs-validator
1efc276f4ff9: Already exists 
a2f2f93da482: Already exists 
12cca292b13c: Already exists 
69e15dccd787: Already exists 
4f4fb700ef54: Already exists 
Digest: sha256:b796fee1fc8e99d857ab1fd6f6438af7d20c3dba74af4d1934f609c22e4dc1ac
Status: Downloaded newer image for ghcr.io/mobilitydata/gtfs-validator:latest
ghcr.io/mobilitydata/gtfs-validator:latest

$ docker run --rm ghcr.io/mobilitydata/gtfs-validator:latest --help
Error: Unable to access jarfile gtfs-validator-cli.jar

I've made the following changes in this PR to both fix the issue and add a testing step that would catch breaks in the Docker layer:

  • Applied the pattern recommended by the current build-push-action for testing the Docker image before pushing
  • Adjusted the CLI to return exit code 0 when invoked with --help
    • Needed a simple command to run that would return 0 if the executable is healthy
    • Other available arguments require valid input
    • It is conventional for --help to return 0 and be used to verify you can run a command, the README currently provides it as such
    • main/parseArguments have a weird split of responsibilities currently. I tried a few ways to keep System.exit() within main as it is in all other cases, but they all ended up getting more gnarly. parseArguments is already handling direct output and having side effects, so is already not a pure function.
  • Fix the path in Dockerfile to the new CLI jar build output location
  • Adds some more ancillary trees to .dockerignore to improve build caching when iterating locally

Copy link
Collaborator

@bdferris-v2 bdferris-v2 left a comment

Choose a reason for hiding this comment

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

LGTM! Apologies I missed this in the previous PR. Thanks for adding a test to catch this in the future.

@isabelle-dr isabelle-dr merged commit 0923169 into MobilityData:master Aug 17, 2022
@isabelle-dr
Copy link
Contributor

Thanks for catching this @themightychris !
Merging, thanks for providing the code review here @bdferris-v2

cc @maximearmstrong

@themightychris themightychris deleted the fixes/docker-cli-path branch August 17, 2022 16:46
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

3 participants