Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Switch to GitHub Actions #143

Merged
merged 19 commits into from
Sep 15, 2021
Merged

Switch to GitHub Actions #143

merged 19 commits into from
Sep 15, 2021

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented Feb 19, 2021 •
edited
Loading

This PR adds an example workflow for implementing the test runner using GitHub Actions.

@desrosj desrosj self-assigned this Feb 19, 2021
pfefferle
pfefferle reviewed Feb 20, 2021
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
pfefferle added 4 commits May 19, 2021 10:24
@pfefferle
generate .ssh folder if not existent
3358d55
@pfefferle
re-add travis file
d551cae 
the hoster should be able to choose between different ways to run the `phpunit-test-runner`
@pfefferle
add scheduler example
66ced63
@pfefferle
add env vars as example
cb4576c
@pfefferle pfefferle self-assigned this May 19, 2021
pfefferle added 2 commits May 19, 2021 10:50
@pfefferle
Add continue-on-error: true for phpunit
c2fe22c 
Otherwise the reporting will be skipped if phpunit runs into errors.
@pfefferle
Add some doc
9972040
@pfefferle pfefferle marked this pull request as ready for review May 21, 2021 08:34
@pfefferle pfefferle requested a review from getsource May 21, 2021 08:34
@pfefferle
Copy link
Member

@desrosj can you have a look at the latest changes?

desrosj
desrosj commented Jul 7, 2021
View reviewed changes
Copy link
Contributor Author

@desrosj desrosj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for running with this, @pfefferle! I haven't looked into the reason why the workflow run is failing, but left some general feedback for the workflows themselves.

.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated
steps:
- name: Cancel previous runs of this workflow (pull requests only)
if: ${{ github.event_name == 'pull_request' }}
uses: styfle/[email protected]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 3rd party actions, using version numbers and ranges is OK, but there are some tradeoffs. The Security hardening page in the GHA docs details these.

In Gutenberg and WordPress Core, we have adopted the pattern of using the full length commit SHA followed by an inline # v0.8.0 comment.

@pfefferle
Copy link
Member

@desrosj the test fails, because it is not configured properly. This repo should only be a whitepaper/example for hosters, how they can run the unit-tests with different action or CI/CD services.

pfefferle and others added 9 commits August 4, 2021 10:19
@pfefferle
Revert "change caching"
1631b97 
This reverts commit 6080a4e.
@desrosj
Use GHA native concurrency to cancel duplicate workflow runs.
cdbe3f5
@desrosj
Use caching built into actions/setup-node.
aabc6d4
@desrosj
Use commit SHAs for third-party actions.
a47082b 
This follows the GitHub Actions security best practices when using third-party actions. For more info, see https://docs.github.com/en/actions/learn-github-actions/security-hardening-for-github-actions#using-third-party-actions.
@desrosj
Improve documentation.
dcc7b2b
@desrosj
Change the schedule example to match the suggestions on https://mak…
eddcad5 
…e.wordpress.org/hosting/test-results-getting-started/.
@desrosj
More documentation improvements.
ec143fa
@desrosj
Remove Remove push and pull_request events.
7c72203
@desrosj
Add a condition to disable the workflow.
01652cb 
GHA will not be the preferred way to run the tests for everyone. This includes a condition to disable it by default.
@desrosj
Copy link
Contributor Author

desrosj commented Aug 18, 2021

@pfefferle I've made a handful of updates to polish the workflow. I think it's in a great state. I do think there is one thing we need to discuss before merging.

Since the workflow is an example for how hosts can run the tests using GHA, I think it should be disabled by default. I've added a condition in the workflow to never run, and documented that the implementer should remove that if GHA is preferred.

@pfefferle
Copy link
Member

@desrosj I have problems with the caching of actions/setup-node:

Found in cache @ /opt/hostedtoolcache/node/14.17.6/x64
/opt/hostedtoolcache/node/14.17.6/x64/bin/npm config get cache
/home/runner/.npm
Error: Dependencies lock file is not found in /home/runner/work/***/***. Supported file patterns: package-lock.json,yarn.lock

@pfefferle
Copy link
Member

Since the workflow is an example for how hosts can run the tests using GHA, I think it should be disabled by default. I've added a condition in the workflow to never run, and documented that the implementer should remove that if GHA is preferred.

I like the idea! 👍

@desrosj
Copy link
Contributor Author

desrosj commented Sep 13, 2021

@desrosj I have problems with the caching of actions/setup-node:

Found in cache @ /opt/hostedtoolcache/node/14.17.6/x64
/opt/hostedtoolcache/node/14.17.6/x64/bin/npm config get cache
/home/runner/.npm
Error: Dependencies lock file is not found in /home/runner/work/***/***. Supported file patterns: package-lock.json,yarn.lock

Ah, yes a lock file must be present for the NPM caching to configure correctly. But I'm not sure this can be configured because the npm install step happens in the middle of prepare.php. For now, I'm thinking we just remove the caching aspect.

@desrosj
Remove npm caching from the example GHA workflow.
bc5bd6d 
This is difficult to set up because a `package-lock.json` file is required in order to create a cache key. In this context, the lock file should be the one from WordPress Core. But since WordPress is not checked out until the `prepare.php` step, caching cannot be configured before hand.
@desrosj
Copy link
Contributor Author

desrosj commented Sep 13, 2021

Updated! I think this one is looking pretty good!

@pfefferle
Copy link
Member

works like a charm 👍

pfefferle
pfefferle approved these changes Sep 14, 2021
View reviewed changes
Copy link
Member

@pfefferle pfefferle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pfefferle pfefferle merged commit 9957fe4 into master Sep 15, 2021
@pfefferle
Copy link
Member

Thanks a lot @desrosj !

@pfefferle pfefferle deleted the switch-to-github-actions branch September 15, 2021 09:31
@getsource
Copy link
Member

Thanks so much @pfefferle and @desrosj !! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfefferle pfefferle pfefferle approved these changes

@getsource getsource Awaiting requested review from getsource

Assignees

@pfefferle pfefferle

@desrosj desrosj

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@desrosj @pfefferle @getsource