-
Notifications
You must be signed in to change notification settings - Fork 15
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
Skip deploy
workflow when PyPI token is not defined in GitHub secrets
#147
Conversation
Use intermediary job as secrets cannot be directly referenced in `if:` conditionals; see https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow
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.
Excellent work!
However, in my testing, git describe --always
falls back to HEAD
in case there are no tags, meaning that in such a case we will never lint any file. I understand this is better than failing, but I believe we can come up with a better alternative in case there are no tags, such as using the GitHub-provided PR target, or maybe git rev-list --max-parents=0 HEAD
to get the root commit and thus lint all files.
.github/workflows/deploy.yml
Outdated
if "${GITHUB_WORKSPACE}/.github/has-functional-changes.sh" ; then | ||
echo "status=success" >> $GITHUB_OUTPUT |
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 we go multiline, let's do it all the way 😉
if "${GITHUB_WORKSPACE}/.github/has-functional-changes.sh" ; then | |
echo "status=success" >> $GITHUB_OUTPUT | |
if "${GITHUB_WORKSPACE}/.github/has-functional-changes.sh" | |
then echo "status=success" >> $GITHUB_OUTPUT |
.github/workflows/deploy.yml
Outdated
- name: Check PYPI Token | ||
id: check_token | ||
run: | | ||
if [[ -n "${{ secrets.PYPI_TOKEN }}" ]]; then |
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.
The comment you made in the commit to explain the implementation is really good, but it would be useful to also have it as a comment in the code to save some future smartass 😉
We will need @sandcha to create a new PyPI token too, to set the new |
Co-authored-by: Matti Schneider <matti@openfisca.org>
Co-authored-by: Matti Schneider <matti@openfisca.org>
82a031d
to
4253f37
Compare
4253f37
to
269240f
Compare
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.
Beautiful 🤩
Let's merge without PYPI_TOKEN
defined to confirm that deployment is cleanly skipped, and then define it and re-run the job to confirm that deployment is activated 🙂
deploy
workflow when PyPI token is not defined in GitHub secretsPYPI_TOKEN_OPENFISCA_BOT
used indeploy
workflow toPYPI_TOKEN