Skip to content

feat: GitHub Actions docker files CI #1194

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

Merged
merged 3 commits into from
Sep 17, 2020

Conversation

nschonni
Copy link
Member

@nschonni nschonni commented Jan 11, 2020

Took @LaurentGoderre approach to the update.sh script generating out the Travis-CI file and converted it to create separate GitHub Actions.

  • The benefit of this over the Travis diff checking, is the path approach limits the matrix to only queue jobs when a particular Dockerfile is touched.
  • I added the DO NOT MERGE to this because I included a junk commit at the end to show the matrix getting triggered when the files are touched.
  • This does not convert the existing deploy setup from Travis-CI
  • Needed to remove the -it flag from the BATS testing since that doesn't seem to work on Actions
  • Does not remove the CI job when a variant/version is removed currently. EX: like the 8 EOL

Copy link
Member

@SimenB SimenB 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 super exciting! 😀 We need to move the deploy as well (or keep the current test runs as well until we're ready to drop travis) - now deploy will run even if a test fails. It only runs on master, and I think we're unable to merge if any steps fail, but I'd like to be safe

@SimenB SimenB requested a review from a team January 11, 2020 10:58
@nschonni
Copy link
Member Author

Right now, the targets are only setup to run on pull_request and not push, so there aren't any tests for the master push.
The only think I think might make sense to go with this would be to enable the repo setting of "branch is up to date". EX: you get the annoying "Update" button when ever a different PR is merged, but then only things tested agains master can land.

@LaurentGoderre
Copy link
Member

With this, we loose the ability to build branches and there's no longer a build on merges?

@LaurentGoderre
Copy link
Member

LaurentGoderre commented Jan 13, 2020

I like the approach but I think it still should merge on branches and merges.

@nschonni
Copy link
Member Author

They rules could be changed to run on master, but I think making the branch "strict" https://help.github.com/en/github/administering-a-repository/types-of-required-status-checks would give the same result. It's less of an issue with hitting the "Update" button with this, since only affected images would rebuilt, rather than all the jobs

@nschonni nschonni force-pushed the github-actions-docker-files branch from 05509fc to e858f6b Compare January 18, 2020 23:14
@nschonni nschonni changed the title [DO NOT MERGE] feat: GitHub Actions docker files CI feat: GitHub Actions docker files CI Jan 18, 2020
@nschonni
Copy link
Member Author

I've popped off the testing commit so this can land if there is agreement

@nschonni nschonni force-pushed the github-actions-docker-files branch 2 times, most recently from fbe62a9 to d2f4811 Compare February 20, 2020 05:27
@nschonni nschonni force-pushed the github-actions-docker-files branch 2 times, most recently from 11da232 to 8594152 Compare March 12, 2020 05:46
@nschonni
Copy link
Member Author

@LaurentGoderre I added back the build on push for now, since the publish script is relying on the merge commit right now. Also added the build and test scripts as triggers for the testing. It caught my Chakracore mistake, and I've added the patch here if it doesn't land in 13.11 PR first

@LongLiveCHIEF
Copy link

What mechanism are you guys using to actually push images to docker hub? I've seen you mention travis, but when I look at the script in the travis action for the deploy stage, all it does is build as far as I can tell.

I recently took over as the primary maintainer of the octoprint/octoprint official image, and I'm really enjoying the way you guys are doing things, but I'm just looking to connect the dots for that final step.

Are you doing the final publishing of images via auto builds on docker cloud?

Love your work, and appreciate any insight you can give me about the image publish step of your overall workflow!

@SimenB
Copy link
Member

SimenB commented Mar 27, 2020

What mechanism are you guys using to actually push images to docker hub?

The bot opens a PR to the official images, which ends up pushing to docker hub. E.g. the latest: docker-library/official-images#7697

@LongLiveCHIEF
Copy link

Of course! Can't believe I missed that since you guys are an official image it would have to be in docker-library/official-images repo. Thanks for filling in the blank for me!

@nschonni nschonni force-pushed the github-actions-docker-files branch from 6bf7a4e to 157958c Compare March 27, 2020 22:43
Copy link

@LongLiveCHIEF LongLiveCHIEF left a comment

Choose a reason for hiding this comment

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

have you tried leaving stdin open without attaching it to psuedo-tty? docker run --rm -i node:"$full_tag"?

@nschonni nschonni force-pushed the github-actions-docker-files branch 2 times, most recently from a07e95c to 46589ed Compare March 28, 2020 15:09
@nschonni
Copy link
Member Author

@nodejs/docker do you want to land this before v14?

@SimenB
Copy link
Member

SimenB commented Mar 28, 2020

I see no reason to wait to land this if we think it works - it shouldn't be observable to consumers anyways.

The way it's set up now it'll trigger an upstream PR even if a build fails - is the idea that it'll fail from the PR first anyways, so no broken versions can land on master?

@nschonni
Copy link
Member Author

Yes, but i'll also look at that after this lands. I just didn't want to mix too much together

@nschonni nschonni force-pushed the github-actions-docker-files branch from 46589ed to 8105d38 Compare April 10, 2020 15:13
@SimenB
Copy link
Member

SimenB commented Apr 21, 2020

Needs to be updated for v14.

Should we keep the travis builds for a while to try GH Actions out, and then remove travis if we're happy? I fully expect we'll be happy with it, but who knows 🙂

@nschonni nschonni force-pushed the github-actions-docker-files branch 2 times, most recently from 37b0918 to a7e5306 Compare June 10, 2020 05:09
@nschonni
Copy link
Member Author

Added v14, and dropped v13

@nschonni nschonni force-pushed the github-actions-docker-files branch 2 times, most recently from 7d5c032 to 70a7f7f Compare June 11, 2020 05:35
@SimenB
Copy link
Member

SimenB commented Jun 17, 2020

@nodejs/docker thoughts on this one? I like it, FWIW 🙂

@SimenB SimenB mentioned this pull request Aug 12, 2020
@nschonni nschonni force-pushed the github-actions-docker-files branch from 70a7f7f to c5397b9 Compare August 15, 2020 04:46
@nschonni nschonni force-pushed the github-actions-docker-files branch from c5397b9 to a84ff50 Compare August 15, 2020 04:48
@nschonni
Copy link
Member Author

I think I'll go ahead an land this sometime after the security releases are out and are successfully published next week

@nschonni
Copy link
Member Author

I'll land this tomorrow unless anyone from @nodejs/docker has any objections

@SimenB
Copy link
Member

SimenB commented Sep 17, 2020

I'm still hugely in favour 👍

@nschonni nschonni merged commit 3a5c226 into nodejs:master Sep 17, 2020
@nschonni nschonni deleted the github-actions-docker-files branch September 17, 2020 21:38
PeterDaveHello added a commit to PeterDaveHello/docker-node that referenced this pull request Sep 21, 2020
The job name was duplicated with the top level name.
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.

5 participants