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.

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

GitHub Actions workflow for Linux and Windows machines #4251

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mickeygousset
Copy link

@mickeygousset mickeygousset commented Apr 24, 2020

Edit: added @tspascoal as a co-author on this.

@tspascoal and I have created a GitHub Actions workflow that will build express on both Linux and Windows machines. I have not opened an issue to link to this, in order to allow all discussion to happen on the PR. I'm happy to open an issue if someone would like me to.

Average build time for Travis (Linux) and AppVeyor(Windows) pipelines currently in use is around 30 minutes. With this workflow, I was able to build both Linux and Windows in just over 5 minutes, total. This is in part due to the number of concurrent pipelines that GitHub gives to open source projects.

If you look at the file, you'll notice I am using the same steps for both the Linux builds and the Windows builds. This allows for code reuse, and also makes it where, if you need to modify a step, you only have to modify it in one place.

I'm making use of a matrix strategy as well. Right now, the workflow is set to only use Linux and Windows. If we wanted to also build on Mac (which is an option), all we would need to do is add macos-latest to the matrix. The same with any other specific versions of Node we would want to test against.

Here is an example of the build output.

Here is an example of a specific job (Node 2.5 running on Linux)

You can click into the jobs and then into each individual step on a job and see details of the run.

At this point, this workflow does everything the current Travis and Appveyor workflows do. In addition, it goes a step beyond the AppVeyor workflow, because the GH Action workflow supports pulling down the nightly build for the Windows build (similar to what the Travis build does), which the current Appveyor workflow does not do.

This is the initial version of the GH Action workflow for building expressjs/express on Linux and Windows
@jonchurch
Copy link
Member

Related expressjs/discussions#88

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Hey! Awesome work! One request, could you take us through some of the decisions in here? It would be super helpful to have all of it as comments in the code for future folks.

.github/workflows/express-build.yml Outdated Show resolved Hide resolved
.github/workflows/express-build.yml Outdated Show resolved Hide resolved
.github/workflows/express-build.yml Outdated Show resolved Hide resolved
.github/workflows/express-build.yml Outdated Show resolved Hide resolved
.github/workflows/express-build.yml Outdated Show resolved Hide resolved
.github/workflows/express-build.yml Outdated Show resolved Hide resolved
.github/workflows/express-build.yml Outdated Show resolved Hide resolved
mickeygousset and others added 5 commits April 24, 2020 13:40
Changed the name of the workflow from ExpressJS Express Build to Test, per feedback from team.

Co-Authored-By: Wes Todd <wes@wesleytodd.com>
Renamed YAML file to Test.yml, based off feedback from the initial pull request.
Changed the workflow to be any pull request to any branch, per suggestions from the submitted pull request. Leaving "on push to master" in right now, to facilitate testing, and in the event someone does do a push directly to master.
- Added Node versions 13 and 14 to the matrix
- Commented out nightly build from the matrix
- Added comments to YAML file
- Change to bash script that runs on all platforms, for calculating the major node versions
- removed if statement from the prune and rebuild node_modules step.
- Add more comments to the file
@mickeygousset
Copy link
Author

At this point I've made all the initially requested changes from the comments earlier in the pull request.

.github/workflows/Test.yml Outdated Show resolved Hide resolved
Changed the matrix to a list format and added the nightly builds.
Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Looks good. Still feels a bit weird to merge this only to follow up with adding node@14 and moving 13 off experimental, but if LGTM then do that addition.

@mickeygousset
Copy link
Author

So I have a "process" question. For all the conversations above in this PR, should I mark them as resolved, or should someone else? I think I've answered all the questions asked, but didn't know the appropriate protocol. @wesleytodd @dougwilson could you provide me some guidance?

@dougwilson
Copy link
Contributor

Hi @mickeygousset I didn't even know you had the ability to mark them as resolved 😄 . Yes, please do so! Wes and myself spoke about this issue last night and I mentioned resolving the conversations to clear it up, but if you want to do so, please go ahead. Wes gave his blessing there at the end, so I believe this is ready to land at this point. I'm targeting it into the 4.18 branch to get in the line prior to release, which will be a huge improvement to testing times!

One thing I started looking at (but never really finished) is what we need to change on our badges to get them to show the status of these GitHub Actions now vs the Travis CI + AppVeyor statues. Not sure if you have that knowledge, and I'm still going to look into it, but just wanted to mention it here in case you knew ❤️

@mickeygousset
Copy link
Author

mickeygousset commented May 21, 2020

I don't know if I have the ability, but it gives me a Resolve button, so I will try. :)

Yes, there are status badges. I'll figure it out and report back.

@mickeygousset
Copy link
Author

I have resolved everything I have a button to resolve with :)

@mickeygousset
Copy link
Author

I think @dougwilson may still have to do something to clear up the blocking.

@dougwilson
Copy link
Contributor

Nice on the badge! I'm looking at our badge provider (badgen.net) and I think the way we would do it would be to use the https://badgen.net/github#github-status-owner-repo-ref-context area, where the name of the status check that the workflow leaves would be what we would key off.

@dougwilson
Copy link
Contributor

Ok, so looking at badgen/badgen.net#304 it seems that we could use https://badgen.net/github#github-checks-owner-repo-ref for the exact replica of that github workflow badge link through badgen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants