X Tutup
The Wayback Machine - https://web.archive.org/web/20200917205721/https://github.com/nodejs/docker-node/pull/1194
Skip to content

/ docker-node

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

feat: GitHub Actions docker files CI #1194

Open
wants to merge 3 commits into
base: master
from
Open

feat: GitHub Actions docker files CI #1194

wants to merge 3 commits into from
+577 −244

Conversation

@nschonni
Copy link
Member

nschonni commented Jan 11, 2020 •
edited

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
@SimenB
SimenB approved these changes Jan 11, 2020
View changes
Copy link
Member

SimenB left a comment

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 nodejs/docker Jan 11, 2020
@nschonni
Copy link
Member Author

nschonni commented Jan 11, 2020

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
Contributor

LaurentGoderre commented Jan 13, 2020

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

LaurentGoderre commented Jan 13, 2020 •
edited

I like the approach but I think it still should merge on branches and merges.
@nschonni
Copy link
Member Author

nschonni commented Jan 13, 2020

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 nschonni:github-actions-docker-files branch from 05509fc to e858f6b Jan 18, 2020
@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

nschonni commented Jan 18, 2020

I've popped off the testing commit so this can land if there is agreement
@PeterDaveHello
PeterDaveHello reviewed Jan 20, 2020
View changes
test-image.bats Outdated Show resolved Hide resolved
@PeterDaveHello
PeterDaveHello reviewed Jan 20, 2020
View changes
.github/workflows/10-alpine3.11.yml Show resolved Hide resolved
@nschonni nschonni force-pushed the nschonni:github-actions-docker-files branch 2 times, most recently from fbe62a9 to d2f4811 Feb 20, 2020
@nschonni nschonni force-pushed the nschonni:github-actions-docker-files branch 2 times, most recently from 11da232 to 8594152 Mar 12, 2020
@nschonni
Copy link
Member Author

nschonni commented Mar 12, 2020

@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
@nschonni nschonni mentioned this pull request Mar 12, 2020
Closed
@LongLiveCHIEF
Copy link

LongLiveCHIEF commented Mar 27, 2020

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

LongLiveCHIEF commented Mar 27, 2020

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 nschonni:github-actions-docker-files branch from 6bf7a4e to 157958c Mar 27, 2020
@LongLiveCHIEF
LongLiveCHIEF reviewed Mar 28, 2020
View changes
Copy link

LongLiveCHIEF left a comment

have you tried leaving stdin open without attaching it to psuedo-tty? docker run --rm -i node:"$full_tag"?
test-image.bats Show resolved Hide resolved
@nschonni nschonni force-pushed the nschonni:github-actions-docker-files branch 2 times, most recently from a07e95c to 46589ed Mar 28, 2020
@nschonni
Copy link
Member Author

nschonni commented Mar 28, 2020

@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

nschonni commented Mar 28, 2020

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 nschonni:github-actions-docker-files branch from 46589ed to 8105d38 Apr 10, 2020
@SimenB
SimenB reviewed Apr 10, 2020
View changes
.github/workflows/10-alpine3.10.yml Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Apr 21, 2020 •
edited

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 nschonni:github-actions-docker-files branch 2 times, most recently from 37b0918 to a7e5306 Jun 10, 2020
@nschonni
Copy link
Member Author

nschonni commented Jun 10, 2020

Added v14, and dropped v13
@nschonni nschonni force-pushed the nschonni:github-actions-docker-files branch 2 times, most recently from 7d5c032 to 70a7f7f Jun 11, 2020
@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
Open
@SimenB
SimenB reviewed Aug 12, 2020
View changes
update.sh Outdated Show resolved Hide resolved
@nschonni nschonni force-pushed the nschonni:github-actions-docker-files branch from 70a7f7f to c5397b9 Aug 15, 2020
nschonni added 3 commits Aug 15, 2020
@nschonni
chore: Generate Docker file GitHub Actions
42a70be
@nschonni
chore: Switch pipeline generation test to Actions
867a567
@nschonni
fix: Remove -it from BATS testing
Loading status checks…
a84ff50 
Interactive shell doesn't seem to work on Actions
@nschonni nschonni force-pushed the nschonni:github-actions-docker-files branch from c5397b9 to a84ff50 Aug 15, 2020
@nschonni
Copy link
Member Author

nschonni commented Sep 10, 2020

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

nschonni commented Sep 16, 2020

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 👍
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimenB SimenB

@LongLiveCHIEF LongLiveCHIEF

@PeterDaveHello PeterDaveHello

@pesho pesho

@Starefossen Starefossen

@LaurentGoderre LaurentGoderre

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@nschonni @LaurentGoderre @LongLiveCHIEF @SimenB @PeterDaveHello
You can’t perform that action at this time.
X Tutup