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: Failing jobs #17

Merged
merged 2 commits into from
Aug 29, 2022
Merged

Fix: Failing jobs #17

merged 2 commits into from
Aug 29, 2022

Conversation

Meldiron
Copy link
Contributor

@Meldiron Meldiron commented Aug 29, 2022

Currently, we return exit code 1 when the existing dataset is up to date. That is a problem for GitHub since it considers such a test failed and shoots notifications at us.

This PR makes sure the job exits successfully even when our data is up to date.

Tested the edge case on a fork:
CleanShot 2022-08-29 at 09 58 25@2x

@@ -1,6 +1,7 @@
on:
schedule:
- cron: "12 3 * * *"
workflow_dispatch:
Copy link
Contributor Author

@Meldiron Meldiron Aug 29, 2022

Choose a reason for hiding this comment

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

This line allows us to trigger jobs manually if ever needed. That becomes really helpful in the future, as it will allow us to test jobs - execute them from a specific branch.

The downside might be security - a person with access to workflow dispatching and making new branches might be able to get hands on some secrets. Would love approval from @eldadfux on this. If not secure, we can revert it.

CleanShot 2022-08-29 at 09 55 38@2x

Copy link
Member

Choose a reason for hiding this comment

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

As long as forks don't get access to it, we should be fine.

Copy link
Contributor Author

@Meldiron Meldiron Aug 29, 2022

Choose a reason for hiding this comment

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

Forced a push on my fork. It was successful, but it pushed to my fork. Looks like we are using some pre-defined secrets for that, so it should be all safe.

@eldadfux eldadfux merged commit d65fcf7 into master Aug 29, 2022
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.

2 participants