-
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
fix: Modify end_to_end.yml to be more specific about which gtfs-validator jar is used #1131
fix: Modify end_to_end.yml to be more specific about which gtfs-validator jar is used #1131
Conversation
…jar should be used, as we think we've run into an edge-case where there are multiple jars in the main/build/libs directory (maybe from a previous build?) that caused the script to fail. See issue MobilityData#1129 for discussion.
|
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.
These changes LGTM, although the same pattern exists in end_to_end_big.yml and end_to_end_100.yml as well. I'd suggest we change all these occurrences to be consistent.
…use. We think we've run into an edge-case where there are multiple jars in the main/build/libs directory (maybe from a previous build?) that caused the script to fail. See issue MobilityData#1129 for discussion.
@barbeau good call. 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.
Thanks @bdferris-v2, LGTM!
The acceptance tests are still blocked by #1094, so if we want them to run before merge this PR would need to be merged after PR #1102.
All the end_to_end*.yml tests look to be passing, which I think is the most important thing to check here. So I propose to submit without waiting for #1102. I will do so this evening unless I hear objections. |
But maybe you were instead pointing out that I won't be able to submit this PR until #1102 is in, regardless? |
No objections here. I think the repo permissions allow merge even with failing/canceled tests. |
Thank you for this contribution @bdferris-v2 and @barbeau ! We didn’t get a chance to add this to the contribution guidelines yet, but can you make sure there is a review from one MobilityData staff before merging please? 🙂 cc @isabelle-dr |
Per discussion in issue #1129, I think I ran into an edge-case where there are multiple jars in the main/build/libs directory (maybe from a previous build?) that caused the script to fail. Attempting to fix by being more specific about which of the :main project jars to use (hint: we want the shadow jar).