-
Notifications
You must be signed in to change notification settings - Fork 61
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
Weekly cron job checking for submodule updates #1741
Weekly cron job checking for submodule updates #1741
Conversation
@@ -0,0 +1,11 @@ | |||
#!/usr/bin/env bash | |||
|
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.
Consider adding set -euxo pipefail
(https://vaneyckt.io/posts/safer_bash_scripts_with_set_euxo_pipefail)
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 for this use-case it isn't what we want, because it would exit the script and Github Action early and we would not get output for any submodules that were not checked yet. We want to exit late, not early, right?
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 would still put it. We want to exit whenever a command fails. Consider 4 fails (silently). Then we would get an empty diff.
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.
add a small summary of what the script does.
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.
diff
returns nonzero upon any difference. So no, here we don't want to exit upon any 'fail', we'd ever only get the difference reported for a single module.
I'll add a summary!
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.
You're right (Cf. http://mywiki.wooledge.org/BashFAQ/105). But what about error handling, e.g., if git submodule foreach 'git fetch'
fails?
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.
When I disable network and run the script, the fetch
loop terminates early (after the first remote being inaccessible) and the diff is generated based on unupdated module remotes. That is not terribly useful, but also not terribly bad, assuming that by running this weekly, such a failure every now and then will do no more harm than postpone us being notified a week late of updates.
Is that acceptable?
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.
Some minor comments inline
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
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.
If there are differences between local and upstream submodules, and the automatically generated issue is not addressed, does that mean that every week an identical issue will be opened?
scripts/submodule-diff.sh
Outdated
#!/usr/bin/env bash | ||
# Check any git submodule remotes for updates, and print difference with current Arbor repo state to `diff.log` | ||
|
||
git submodule foreach 'git describe HEAD --tags' > current_state_of_git_submodules_in_arbor_repo.log | tee current_state_of_git_submodules_in_arbor_repo.log |
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.
Why do we need both | tee
and >
?
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.
To both display the output on stdout and have it in the file.
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 just tried it out locally and it didn't display the output on stdout. But git submodule foreach 'git describe HEAD --tags' | tee current_state_of_git_submodules_in_arbor_repo.log
does what you describe.
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.
Whoops :)
Correct. |
I think a new issue every week might be too much, what do you think? |
Weekly is perhaps too frequent, I've changed it to a monthly job. |
diff.log
if git submodules have tags more recent that the commit they're currently at.First step in addressing #1731