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

Switch to GitHub Actions #531

Merged
merged 13 commits into from Aug 1, 2020
Merged

Switch to GitHub Actions #531

merged 13 commits into from Aug 1, 2020

Conversation

lhr0909
Copy link
Contributor

@lhr0909 lhr0909 commented Jun 21, 2020

Resolves #528

  • Run cargo test with a NodeJS version matrix and Rust toolchain matrix
  • Specific environment dependencies (apt dependencies etc)
  • Electron settings
  • Generate Documentation
  • Windows build
  • MacOS build

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jun 21, 2020

Currently build is being run on my fork: https://github.com/lhr0909/neon/actions

@amilajack
Copy link
Member

Your CI seems to be failing on linux builds because its timing out. Any ideas for why that might be?

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 8, 2020

https://github.com/lhr0909/neon/actions/runs/143132073 I think I resolved that @amilajack I believe it failed because of the electron set up wasn't there.

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 9, 2020

https://github.com/lhr0909/neon/actions/runs/163149424 docs workflow is ready to go now.

@@ -0,0 +1,28 @@
name: Generate Docs
Copy link
Member

Choose a reason for hiding this comment

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

@dherman @amilajack Do we use Github Pages for docs anymore? I thought we switched to only using docs.rs?

Copy link
Member

Choose a reason for hiding this comment

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

Yea we do. I think the domain is hosted with netlify. But our static content for the site is hosted with gh pages. Cargo generated API docs are hosted on docs.rs.

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 9, 2020

https://github.com/lhr0909/neon/runs/854609942?check_suite_focus=true#step:6:388 I got very close to resolve the Windows build but seems like there is a 60 seconds timeout.


strategy:
matrix:
node-version: [8.x, 10.x, 12.x]
Copy link
Member

@amilajack amilajack Jul 10, 2020

Choose a reason for hiding this comment

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

We're okay with dropping support for node 8. We can change this to [10.x, 12.x, 14.x] and that might make this CI pass. We can also do this for other workflows as well

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 12, 2020

https://github.com/lhr0909/neon/runs/854609942?check_suite_focus=true#step:6:416

https://github.com/lhr0909/neon/runs/862005882?check_suite_focus=true#step:6:417

Actually, after looking into the test further, seems like it is failing at the tests::cli_test, and it doesn't seem to be a consistent repro.

@dherman
Copy link
Collaborator

dherman commented Jul 12, 2020

@lhr0909 I suspect this has to do with some recent Windows build problems (#406, #501), which we think are fixed with #537. I've reviewed #537 today and we should be able to merge it soon, and then let's see if that just makes these issues here go away. :)

@dherman
Copy link
Collaborator

dherman commented Jul 12, 2020

Getting excited……………

anna-excited2

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 12, 2020

@dherman Thanks Dave! Let me know if we would like to do the MacOS workflow, I can try to set it up.

@dherman
Copy link
Collaborator

dherman commented Jul 13, 2020

@lhr0909 A macOS workflow would be great, yes! 😄

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 18, 2020

https://github.com/lhr0909/neon/actions/runs/173784987 I just rebased on latest master and seems like the Windows test passed.

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 18, 2020

image

Tests pass. Going to remove Appveyor and Travis from my branch.

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 18, 2020

I think I have removed Appveyor file from the branch, but not sure why there is still a check

@lhr0909 lhr0909 changed the title [WIP] Switch to GitHub Actions Switch to GitHub Actions Jul 18, 2020
@kjvalencik
Copy link
Member

Appveyer will continue to build until it's merged.

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This is incredible. It's so readable and simple, and consistent across all three platforms. Amazing work!

I'm approving but it would be good to get @kjvalencik's review as well, since he's worked on our CI in the past as well.

- name: Push docs to gh-pages branch
uses: JamesIves/github-pages-deploy-action@releases/v3
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious how this works. Does GitHub automatically give you this, or did we already have a secret called GITHUB_TOKEN installed somewhere in the repo settings? I couldn't find anything in the repo's settings tab.

Copy link
Member

Choose a reason for hiding this comment

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

GitHub automatically gives it to you. It has limited permissions and a short TTL. It's one of the best features of actions. ❤️

@dherman dherman requested a review from kjvalencik July 18, 2020 20:36
@amilajack
Copy link
Member

Is it possible to consolidate these configs into a single config?

@lhr0909
Copy link
Contributor Author

lhr0909 commented Jul 19, 2020

@amilajack I can make them into a single config, just that we will still end up having this long list of checks because of the node version matrix, rust toolchain versions, and platforms. Also, the conditional logic in a single config will be hard to maintain (for example, docs and xvfb setup on Linux for Electron test). Separate files are easier if we are going to see this long list of checks anyway.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/linux.yml Outdated Show resolved Hide resolved
lhr0909 and others added 2 commits August 1, 2020 10:39
Co-authored-by: K.J. Valencik <kjvalencik@gmail.com>
Co-authored-by: K.J. Valencik <kjvalencik@gmail.com>
@dherman
Copy link
Collaborator

dherman commented Aug 1, 2020

@lhr0909 Thanks for updating! It looks like there are a couple of build failures. I think the Appveyor one doesn't matter since we'll disable it. Do the Actions failures matter or do you think we can merge?

@kjvalencik
Copy link
Member

kjvalencik commented Aug 1, 2020

@dherman It looks like it might be a legitimate test failure on node 14 + windows and not related to actions. The others are cancelled builds because of that failed build.

@kjvalencik
Copy link
Member

@dherman Looks like the failed build is the neon new bug that @amilajack's PR fixes.

I think this is good to merge.

@kjvalencik kjvalencik merged commit f70f4b8 into neon-bindings:master Aug 1, 2020
@kjvalencik
Copy link
Member

@lhr0909 Thanks for rh awesome contribution! I'm exited about this build matrix!

@dherman
Copy link
Collaborator

dherman commented Aug 1, 2020

Thank you @lhr0909!!

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

Successfully merging this pull request may close these issues.

Move CI to GitHub Actions
4 participants