-
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
ci: Remove upload of validation reports to Google Cloud #1102
Conversation
Thank you for this contribution! 🍰✨🦄 Information about source corruption0 out of 1248 sources are corrupted. Acceptance test detailsThe changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase. |
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 @lionel-nj! Comments in-line.
@@ -186,16 +186,6 @@ jobs: | |||
with: | |||
name: reports_all | |||
path: ${{ github.sha }}/output | |||
- name: Set up and authorize Cloud |
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.
Could you describe more what we lose by disabling this? Can we still download the reports as linked in the comment generated by the acceptance test (e.g., "Download the full acceptance test report for commit 6e94daa [here] (report will disappear after 90 days)"? If not does that comment need to change?
If it possible to check here if the Action has permission to use the credentials? If so, we could still run this block for branches on this repo, but just not for 3rd party forks.
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.
Could you describe more what we lose by disabling this?
By doing so, we loose the ability to store validation reports on GCP Cloud storage. But it is still possible to download them from the internal storage of github.
Can we still download the reports as linked in the comment generated by the acceptance test (e.g., "Download the full acceptance test report for commit 6e94daa [here] (report will disappear after 90 days)"? If not does that comment need to change?
Yes - this remains possible.
If it possible to check here if the Action has permission to use the credentials? If so, we could still run this block for branches on this repo, but just not for 3rd party forks.
Let me see.
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.
From a quick searchhere and there; it appears that checking the presence of a repo secret in a if
block of a job's step might not be possible. However, it might be possible to include an intermediate step that consists in checking the presence of said secret and store the boolean in an environmental variable, then execute Set up and authorize Cloud
and Upload reports to Google Cloud Storage
conditionally. Unfortunately I will not have the time to finish it 😔.
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.
Looks like this is the issue regarding making execution conditional on secrets - actions/runner#520
Also, I believe the link you have in your original post of "The process of the acceptance tests will solely rely on the internal Github storage in order to avoid the behavior described #755." is pointing to the wrong issue. |
Thank you for this contribution! 🍰✨🦄 Information about source corruption0 out of 1248 sources are corrupted. Acceptance test detailsThe changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase. |
Based on actions/runner#520 (comment) Expected behavior is that it will upload reports to Google Cloud for PRs for branches on the main repo, but skip upload on PRs from forks.
Thank you for this contribution! 🍰✨🦄 Information about source corruption0 out of 1248 sources are corrupted. Acceptance test detailsThe changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase. |
@maximearmstrong @isabelle-dr I've expanded on the work that @lionel-nj started here to effectively skip all validation report uploads to Google Cloud, which should fix acceptance tests on PRs from forks and therefore close #1094. Ideally we'd conditionally upload reports to Google Cloud only if the PR is for a branch on this repo (and not upload if the PR is from a fork). However, this IF statement in the GitHub Action isn't currently working as expected. Given that the acceptance tests running on forks is a critical part of the workflow of this repo for evaluating PRs like #1125, and uploading reports to Google Cloud is effectively a "nice to have" feature, I propose that we merge this PR as-is to fix the acceptance tests for forks. We can then fix the Google Cloud uploads in future work. I've opened issue #1128 to track re-enabling Google Cloud uploads in the future by tweaking the existing code in this PR. I've also marked the code that's not working as intended with a So as long as all the acceptance tests pass for my latest push to this branch, then as far as I know this should be ok to merge if you agree. |
Thank you for this contribution! 🍰✨🦄 Information about source corruption0 out of 1248 sources are corrupted. Acceptance test detailsThe changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase. |
Thank you for this work @barbeau! @isabelle-dr Do you see value in keeping the reports for more than 90 days? If yes, I agree with Sean that we should merge this PR as-is to fix the problem and then address the Google Cloud issue. If no, I suggest that we remove the code related to Google Cloud right away. |
@maximearmstrong @barbeau I'm ok not keeping the report after 90 days, because:
|
@maximearmstrong @isabelle-dr Sounds good - I've removed the Google Cloud upload code entirely in ced54b9. After the tests run this PR should be ready for review/merge. |
Thank you for this contribution! 🍰✨🦄 Information about source corruption0 out of 1248 sources are corrupted. Acceptance test detailsThe changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase. |
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 @barbeau and @lionel-nj for this contribution :)
closes #1094
Summary:
This PR removes the usage of GCP cloud storage. The content of this PR fixes the behavior described here
Expected behavior:
On forks, acceptance tests should run and not fail because of missing credentials: upload to cloud storage is not used - on this GitHub repository, google cloud storage is used as a backup.
Please make sure these boxes are checked before submitting your pull request - thanks!
[ ] Run the unit tests withgradle test
to make sure you didn't break anything