-
Notifications
You must be signed in to change notification settings - Fork 0
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
add git action for pull requests #129
Conversation
.github/workflows/pull_request.yml
Outdated
@@ -32,22 +32,5 @@ jobs: | |||
ruby-version: "2.6" | |||
bundler-cache: true |
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.
removed the caching steps as the gem action documentation suggests that setting bundler-cache: true will cache your gems (provided they are in the default Gemfile location).
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.
Not a blocker but a suggestion
Since you are touching this, can you pull out the testing into it's own callable workflow file, similar to
the way lupo uses the same CI testing in pull requests/deployment/release
Ex:
https://github.com/datacite/lupo/blob/master/.github/workflows/pull_request.yml
and
https://github.com/datacite/lupo/blob/master/.github/workflows/parallel_ci.yml
(No need to do the stuff to run the specs in parallel here, they are much smaller/faster)
Then we just have one place to update if we need to update something related to tests/specs.
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.
Nice one @wendelfabianchinsamy - we should look at making this change in the workflows on other repos as well. Dealing with the caching stuff has always been a nightmare!
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.
Not a blocker but a suggestion
Since you are touching this, can you pull out the testing into it's own callable workflow file, similar to the way lupo uses the same CI testing in pull requests/deployment/release
Ex: https://github.com/datacite/lupo/blob/master/.github/workflows/pull_request.yml and https://github.com/datacite/lupo/blob/master/.github/workflows/parallel_ci.yml (No need to do the stuff to run the specs in parallel here, they are much smaller/faster)
Then we just have one place to update if we need to update something related to tests/specs.
thanks @jrhoads. this is a great suggestion. I will get on it.
.github/workflows/pull_request.yml
Outdated
@@ -32,22 +32,5 @@ jobs: | |||
ruby-version: "2.6" | |||
bundler-cache: true |
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.
Nice one @wendelfabianchinsamy - we should look at making this change in the workflows on other repos as well. Dealing with the caching stuff has always been a nightmare!
@jrhoads and @digitaldogsbody i've updated the pr with suggestions made by @jrhoads |
.github/workflows/parallel_ci.yml
Outdated
runs-on: ubuntu-latest | ||
strategy: | ||
fail-fast: true | ||
matrix: |
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 matrix is not needed. Since the tests complete in 9 seconds, we don't really gain anything by parallelizing them
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.
thanks @jrhoads. i've updated the pr.
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.
Looks good.
Purpose
Currently there is no git action workflow for pull requests. You need to merge the PR into master in order to ensure that your specs pass. This will allow you to know earlier if there are any failures which can then be fixed before merging into the master branch.
In the future, we can extend this workflow to include the execution of Rubocop, Bundle Audit, Brakeman, etc.
closes: #128
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines: