-
Notifications
You must be signed in to change notification settings - Fork 237
fix: check reqs exist #1835
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: check reqs exist #1835
Conversation
# The branches below must be a subset of the branches above | ||
branches: [ main ] | ||
branches: [main] | ||
schedule: |
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.
TIL how to schedule action runs
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.
fyi, these are... fluffy. don't rely on crons in GHA for precise timing
.github/workflows/main.yml
Outdated
verbose: true | ||
token: ${{ secrets.CODECOV_TOKEN }} | ||
|
||
run-alpine: |
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.
huge. 🥔
action.yml
Outdated
runs: | ||
using: "composite" | ||
steps: | ||
- name: Detect shell |
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 is this necessary? Can't we just use sh
for everything?
action.yml
Outdated
fi | ||
- name: Action version | ||
shell: bash | ||
shell: ${{ env.CC_SHELL }} |
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.
W that all this stuff just works in the less featured shells
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.
1q
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
@@ -1,4 +1,3 @@ | |||
#!/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.
you can do a shebang for sh
- would recommend as otherwise I believe it just uses the default shell, which could potentially be bad? Regardless would rather be explicit about the API necessary for the script to run
Oh I see your commits are called try something lol Ping me on slack when actually need a review. I was going based on GH notifications |
f3f7d62
to
db13bb3
Compare
db13bb3
to
04ec9fe
Compare
steps: | ||
- name: Install all required deps | ||
run: | | ||
apk add git curl gnupg 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.
can we add these as reqs on the README? versions would be useful too
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1835 +/- ##
============================
============================
☔ View full report in Codecov by Sentry. |
An unnecessary breaking change was deployed without updating the major version (codecov/codecov-action#1835). Its web UI still doesn’t work most of the time and its value is limited, so I’d rather just remove it again.
We are requiring users to have the requirements for the action installed ahead of time, and we want to be in control of the error messaging.