-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
[#64] Added GitHub Actions support #117
Conversation
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.
@tannguyen04
Please see my comments inline
.github/workflows/ci.yml
Outdated
branches: | ||
- main | ||
- ci-test | ||
- 9.x |
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.
This is not the same as what in CircleCI's implementation and will not work as required (we need to allow 1.x
, 2.x
etc branches, for example).
GHA does not support regex in branch names as CircleCI does.
But it supports filter patterns: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet
Please update these to support those patterns
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.
Yes. Sorry I forgot inform you on that.
Because the if condition in job does not support regex so I made it simple as I setup in If condition of deploy job. I thought that if user use this project they can update exactly branch name to make GHA work.
But anyway, I have updated the filter of branches for match as CircleCi as your comment now.
Could you please review it ?
Thank you.
.github/workflows/ci.yml
Outdated
if: | | ||
|
||
github.event_name == 'push' && (github.event.ref_type == 'tag' | ||
|| github.event.ref == 'refs/heads/10.x' |
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.
this logic needs to be updated to match the branches wildcards above
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.
Currently, I am stuck with this one as original because If
in Job
does not support regex. So I just leave as simple matching now then user can free to update the condition later.
Or can you give some idea to win this one ?
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.
This is how this could have been done
jobs:
setup:
runs-on: ubuntu-latest
outputs:
isVersionBranch: ${{ steps.check_branch.outputs.isVersionBranch }}
steps:
- id: check_branch
run: |
if [[ "${{ github.ref }}" =~ refs/heads/([0-9]+\.x(-[0-9]+\.x)?|[0-9]+\.[0-9]+\.x) ]]; then
echo "isVersionBranch=true" >> $GITHUB_OUTPUT
else
echo "isVersionBranch=false" >> $GITHUB_OUTPUT
fi
version-specific-job:
needs: setup
runs-on: ubuntu-latest
if: needs.setup.outputs.isVersionBranch == 'true'
steps:
- name: Run version-specific tasks
run: echo "Running tasks for version branches"
After reading your comment
I thought that if user use this project they can update exactly branch name to make GHA work.
I think that the branch naming needs to be updated so that the conusmer's GitHub repo branch is 1.x
and not main
, then the branch on D.O. would be the same.
So, I will merge this as is and will adjust branches accordingly.
Thank you for working on this.
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.
- Remove empty lines. - Remove test code. - Rename GHA.
Checklist before requesting a review
closes #64
[#123] Verb in past tense with dot at the end.
Changed
section about WHY something was done if this was not a normal implementationChanged
Screenshots