-
Notifications
You must be signed in to change notification settings - Fork 287
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
Conversation
Currently build is being run on my fork: https://github.com/lhr0909/neon/actions |
Your CI seems to be failing on linux builds because its timing out. Any ideas for why that might be? |
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. |
https://github.com/lhr0909/neon/actions/runs/163149424 docs workflow is ready to go now. |
@@ -0,0 +1,28 @@ | |||
name: Generate Docs |
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.
@dherman @amilajack Do we use Github Pages for docs anymore? I thought we switched to only using docs.rs
?
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.
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
.
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. |
.github/workflows/linux.yml
Outdated
|
||
strategy: | ||
matrix: | ||
node-version: [8.x, 10.x, 12.x] |
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.
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
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 |
@dherman Thanks Dave! Let me know if we would like to do the MacOS workflow, I can try to set it up. |
@lhr0909 A macOS workflow would be great, yes! 😄 |
https://github.com/lhr0909/neon/actions/runs/173784987 I just rebased on latest master and seems like the Windows test passed. |
I think I have removed Appveyor file from the branch, but not sure why there is still a check |
Appveyer will continue to build until it's merged. |
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 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 }} |
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.
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.
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.
GitHub automatically gives it to you. It has limited permissions and a short TTL. It's one of the best features of actions. ❤️
Is it possible to consolidate these configs into a single config? |
@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 |
Co-authored-by: K.J. Valencik <kjvalencik@gmail.com>
Co-authored-by: K.J. Valencik <kjvalencik@gmail.com>
@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? |
@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. |
@dherman Looks like the failed build is the neon new bug that @amilajack's PR fixes. I think this is good to merge. |
@lhr0909 Thanks for rh awesome contribution! I'm exited about this build matrix! |
Thank you @lhr0909!! |
Resolves #528