-
Notifications
You must be signed in to change notification settings - Fork 169
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
[MISC] Add shellcheck checks to ensure that those few shell scripts we have are "robust" #1774
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1774 +/- ##
=======================================
Coverage 87.93% 87.93%
=======================================
Files 16 16
Lines 1351 1351
=======================================
Hits 1188 1188
Misses 163 163 ☔ View full report in Codecov by Sentry. |
5dd0b11
to
a89b1bf
Compare
oh, actually I have a problem with current pre-commit setup - it requires docker so fails now
keeping it in draft, hopefully would come back to this. |
@effigies @sappelhoff - an easy one to reduce number of open prs by 1 |
do we need more approvals here or could it be merged?? it is just a check to make a world a better place! @effigies do the honors - be the 3rd and press the button? |
Thanks for the ping @yarikoptic, we can merge this with two positive reviews and 5 days passed since the last change. |
Could/should we automate that ? (e.g. 3 positive reviews and 5 days -- auto-merge) |
i would be +1 for this if an additional precondition is that a certain (new) label is added to the PR, like an "automerge" label. Because for some PRs (as this one), an automerge shouldn't be controversial, whereas for some other PRs it might be. |
We have an old issue, maybe closed, about automating this that was never acted upon because no big need, no big motivation to implement. |
drafted one in #1823 for now |
|
TODO