-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat: Initial entry for a workflow to package the gui application. #1166
feat: Initial entry for a workflow to package the gui application. #1166
Conversation
Per https://github.com/MobilityData/gtfs-validator/actions/runs/2382685953, this workflow is successfully packaging for Windows, Mac, and Linux. Ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments before approval. Thanks @bdferris-v2
.github/workflows/package_app.yml
Outdated
- '**/package_app.yml' | ||
release: | ||
types: [ prereleased, released ] | ||
workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the :
here since we are not providing anything after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do need it. I tried removing it and got a workflow error:
https://github.com/bdferris-v2/gtfs-validator/actions/runs/2387663309
More importantly, it seems pretty common to include the :
in other projects:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thank you!
Also, now that #1168 is merged, the acceptance tests should pass for a future commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I'm amazed that this was so straightforward 💯.
I tested the Windows installer that was uploaded and it works correctly 👍.
One small item in-line.
.github/workflows/package_app.yml
Outdated
@@ -0,0 +1,50 @@ | |||
name: Package Application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend renaming this (and file name) so it's more clearly distinguished from other workflows that also "package" the app. Maybe:
name: Package Application | |
name: Package Installer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @bdferris-v2!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small thing aside, we are ready to merge :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we could update README.md in this PR to add the instructions?
We could add a section "Run the validator app" describing (1) how to download the app and (2) how to use it.
If we want to mirror what we have with the JAR file, we will also need to include a DOWNLOAD_SNAPSHOT_APP.md similar to DOWNLOAD_SNAPSHOT_JAR.md.
@isabelle-dr I'm happy to write that documentation, but I think it's getting beyond the original scope of this PR? Would it be ok if I updated documentation in a follow-up PR? |
@bdferris-v2 No problem updating the documentation in another PR, I thought it was in scope. |
We just need to update this branch with |
Done. |
Thank you @bdferris-v2 ! |
Implementation for a GitHub workflow to build the GUI application installers and packages on all key environments.