KhronosGroup / glTF-Project-Explorer

Tool to provide a filterable registry of glTF community projects.
Apache License 2.0
87 stars 46 forks source link

Move to GitHub Actions #164

Closed weegeekps closed 1 year ago

weegeekps commented 1 year ago

This gets us off of Travis CI. Nothing fancy. I had to split the build and deployment steps, but that's nice because we can try the build without the deployment now.

weegeekps commented 1 year ago

Push without a branch specification will mean the build will happen on a push to every branch. This seems wasteful to me but I guess since GitHub Actions is technically free for us since we're open source it'd be okay. Lemme know if you want me to allow builds on all branches.

Ubuntu has been wonky lately, and I no longer trust ubuntu-latest to not break builds. The Ubuntu folks have been making some really major changes between releases lately which have led to a whole host of problems with SSL, glibc, and everything in between. I'd rather pin because it's safer and we can upgrade when a new stable LTS comes out.

NODE_ENV is a standard environment variable used by a lot of node packages for builds and deployments. It's usually a smart idea to set it although with us not currently running the tests I'm unsure if it's being used.

The concurrency stuff is actually there to prevent multiple builds running on top of one another. The docs do a better job explaining this than I could.

Setup Node is actually needed because the Ubuntu that Github actions uses doesn't include node so you can specify a specific version. The action also allows us to set up a cache for yarn so future builds will go faster. Not a huge deal for this project but helpful none the less.

Install Packages actually pulls down the node packages used by our project from NPM, so we will need to run this step. It should go fast after the first build though given the yarn cache that the Node JS action provides us.

weegeekps commented 1 year ago

Eventually, .travis.yml should be removed from this branch.

Also, good catch. Fixing this now.

javagl commented 1 year ago

When you say that a certain line makes sense, then I could spend a while reading documentation and specifications (and probably try to find a balance between contradicting "Best practices" that are proposed in different sources), or just look at the ✔️ and say: "Yeah, seems fine"...

Lemme know if you want me to allow builds on all branches.

I'd have to read more about how the setting/config there translates across forks. Roughly: When someone creates a branch in a fork, then this should be built on every push, just to make it easier to be alerted about anything that may break the build. (It's a trade-off. We don't want to spam GitHub with "unnecessary" builds (even when they are "free"), but ... I think that we won't have "dozens" of forks and branches that people are actively working on, so it's probably not a big deal anyhow...).

For the other points: I know that it's possible to use npm and node without any additional installation of Node on ubuntu-latest. At least, that's what is done in the only GitHub CI file that I've dealt with until now, namely https://github.com/javagl/3d-tiles-validator/blob/main/.github/workflows/ci.yml , which is copied-and-pasted fro... inspired by the one at https://github.com/KhronosGroup/glTF-Validator/blob/main/.github/workflows/dart.yml .

But again, I cannot say what's 'right' or 'wrong' here, beyond "Here's one thing that works, and here's one thing that also works, but is different from the first (so I wonder why)". So I'll assume that the env, concurency and yarn part make sense here, and unless there are further comments or changes, will merge this soon.

weegeekps commented 1 year ago

I'd have to read more about how the setting/config there translates across forks. Roughly: When someone creates a branch in a fork, then this should be built on every push, just to make it easier to be alerted about anything that may break the build. (It's a trade-off. We don't want to spam GitHub with "unnecessary" builds (even when they are "free"), but ... I think that we won't have "dozens" of forks and branches that people are actively working on, so it's probably not a big deal anyhow...).

This is a fantastic point and one I didn't consider. I think this would cause an issue with their fork. I gotta think about this one some. I can see merits to both ways.

javagl commented 1 year ago

The potential issue might be alleviated by the fact that as soon as someone opens a PR here, the build will run (and that's the most important part). Whether people would like to develop in a branch, without having to open a (Draft) PR just in order to trigger a build is hard to say.

javagl commented 1 year ago

It's probably fine to merge the PR in general. Details about the triggers for the build process may also be sorted out later, if necessary.