-
Notifications
You must be signed in to change notification settings - Fork 294
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 actions to build PRs and develop, cleanup closed PRs. #1172
Conversation
@felixarntz someone with repo ownership will need to add a personal access token as a repo secret under the key Also I noticed the build failed, possible because of some recent changes in build commands? I'll take a look and get that fixed. |
Fixed the build command, I had to adjust for the newer Once the |
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.
Awesome work! Looking forward to seeing this in action.
I'll merge after we're done with the current release.
.github/workflows/build-pr.yml
Outdated
npm install gulp-cli -g | ||
- name: Build develop version | ||
run: | | ||
npm run build:dev |
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 really scope of this PR, but the scripts in package.json
are a bit inconsistent between how they work for production vs development. E.g. build:dev
includes a call to build:static
while build:production
and build:test
don't.
I'll open a small separate issue with 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.
This is great @adamsilverstein - I left a few comments, questions and suggestions for you but nothing blocking. Mostly that the code is a bit unclear in some places as to why something is done a certain way which could benefit from clarification via comments and/or better variable names.
I actually created a Docker container recently for running the plugin build in a reproducible way. I'm not sure if that could be used here (it only supports a production build and a specific branch but also supports using a local repo) but might be useful to see if anything can be borrowed there. https://github.com/aaemnnosttv/gskbuild
@aaemnnosttv Thanks for the review, I'll work on addressing your feedback
Very cool! I have a separate tester plugin I am working on to leverage these zip builds, more on that soon! |
…-wp into feature/add-repo-actions
8ad96b3
to
36f4963
Compare
@aaemnnosttv This is ready for re-review, I have addressed your feedback and cleaned things up a bit. Thanks especially for the bash script improvements. We added the required secret and ZIPs are building now (for this pr only so far), you can clone the wiki repo to see them: |
e02e1aa
to
173a64b
Compare
Thanks! |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@adamsilverstein I've made a few updates to the workflow to simplify things a bit which I think you will like!
Let me know what you think or if there's anything I might have missed. |
Requesting @tofumatt's eyes here since he is quite familiar with Actions as well 👍 |
@aaemnnosttv Thanks for the refactoring here!
Nice.
Did you figure out a way to determine or specify an artifact's URL via API or after creation? If we knew this we could use build artifacts instead of the wiki.
Excellent, that is much cleaner and easier to understand as well!
Great! I'll remove my token.
the github actor feels clear enough to me. |
I thought about that but the fact that they expire is a bit of a dealbreaker - also, they're specific to the individual action run, so I don't think that there is an easy way to grab the latest like we are doing with the URL to the wiki. Also, build artifacts are put into a directory even if you upload an individual file - the artifact comes down as a zipped directory. That is, you wouldn't be able to link to an artifact of the installable zip, unless you used |
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.
Seems good to me—I have a question about zips built from develop
, but otherwise this seems great and puts us on the way to having automated releases… and eventually even automatic deploys to WP's plugin repo! 🎉
@@ -0,0 +1,83 @@ | |||
name: build-pr | |||
|
|||
on: |
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 build the PR on any tags/and on master
it would save us (usually @felixarntz) having to build the production ZIP by hand on a Mac. Having the CI generate our builds instead of us doing it manually is way nicer, so I filed #1196 to chat about that and extend this later 😄
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.
Good call!
uses: php-actions/composer@v1 | ||
with: | ||
args: install | ||
- name: Read .nvmrc |
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.
That's a nice touch. I usually just specify the node version in the action, but I like that! 👍
on: | ||
push: | ||
branches: | ||
- develop |
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 runs on pushes to develop
, but doesn't remove the file. Will the upload overwrite this one based on the ref
being the branch name (develop
) over the commit hash?
If so that seems fine, but just wanna make sure this isn't going to spawn a lot of ZIP files 😅
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 generated zip files are renamed as google-site-kit.zip
and google-site-kit-dev.zip
so yes 😄
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.
note: the github.ref in this case is refs/heads/develop
(so that is where the zips wind up being stored)
uses: actions/download-artifact@v1 | ||
with: | ||
name: zip-files | ||
path: ${{ github.ref }} |
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.
Love how this works!
Excellent updates @aaemnnosttv - thank you! +1 |
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 👍
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.
Awesome work @adamsilverstein and @aaemnnosttv!
Summary
This PR adds two GitHub Actions to automatically build (and later cleanup) versions of the plugin for testing. These build files can be used to set the version of Site Kit to any PR or develop for testing.
build-pr
action builds a zip production (google-site-kit.zip
) and development(google-site-kit-dev.zip
) build version of the plugin.refs/pull/[PR#]/merge
develop
branch, in this case the ref isrefs/heads/develop
.remove-pr
action runs and the ref folder and zip files are deleted from the wiki repository.Relevant technical choices
npm
andcomposer
assets (unless they change)