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

ci: use single dynamic build-test action #1339

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

ttshivers
Copy link
Member

Add a GitHub action that dynamically generates the matrix of Dockerfiles to test based on what files have changed. This avoids having to have generated workflows for each version x variant combo.

It looks for any changed Dockerfiles, or docker-entrypoint.sh. If any of the testing files are changed, it will test all Dockerfiles

Copy link
Member

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

This would probably also drop the https://github.com/nodejs/docker-node/blob/master/.github/workflows/dockerfiles.yml
and -c part of the update.sh file

.github/workflows/build-test.yml Show resolved Hide resolved
@ttshivers
Copy link
Member Author

This would probably also drop the https://github.com/nodejs/docker-node/blob/master/.github/workflows/dockerfiles.yml
and -c part of the update.sh file

That makes sense with dropping https://github.com/nodejs/docker-node/blob/master/.github/workflows/dockerfiles.yml. In fact, that file currently calls ./update.sh -t, which isn't even a legal option at the moment.

It doesn't look like there is a -c part of the update.sh. Were you meaning the -b part?

@nschonni
Copy link
Member

It doesn't look like there is a -c part of the update.sh. Were you meaning the -b part?

Right, I forgot the letter I ended up changing it to 😆

Copy link
Member

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

Think this will probably make a good base for the expanded architecture stuff

update.sh Outdated Show resolved Hide resolved
@ttshivers
Copy link
Member Author

ttshivers commented Sep 28, 2020

One question I have is whether to keep around test-build.sh and test-image.bats since the GitHub test action won't use them anymore.

@nschonni
Copy link
Member

One question I have is whether to keep around test-build.sh and test-image.bats since the GitHub test action won't use them anymore.

I don't feel very strongly about keeping them, but I think some people liked them, so I'll see who chimes in

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.

One question I have is whether to keep around test-build.sh and test-image.bats since the GitHub test action won't use them anymore.

I don't feel very strongly about keeping them, but I think some people liked them, so I'll see who chimes in

I wanted to keep them as a way to run a test on the image from the outside, rather than on the inside during build. However, the tests now do docker run so I'm personally happy to remove them

genMatrix.js Show resolved Hide resolved
.github/workflows/build-test.yml Show resolved Hide resolved
.github/workflows/build-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-test.yml Outdated Show resolved Hide resolved
genMatrix.js Outdated Show resolved Hide resolved
@SimenB SimenB requested a review from a team September 28, 2020 08:21
@ttshivers ttshivers force-pushed the single_test_action branch 2 times, most recently from 6c8543e to aa615be Compare September 28, 2020 19:24
genMatrix.js Outdated Show resolved Hide resolved
genMatrix.js Outdated Show resolved Hide resolved
@SimenB SimenB requested review from nschonni and a team October 2, 2020 06:03
@SimenB
Copy link
Member

SimenB commented Oct 8, 2020

One question I have is whether to keep around test-build.sh and test-image.bats since the GitHub test action won't use them anymore.

Just delete those and we can merge? 🙂

@ttshivers
Copy link
Member Author

One question I have is whether to keep around test-build.sh and test-image.bats since the GitHub test action won't use them anymore.

Just delete those and we can merge? 🙂

Deleted those files

@SimenB
Copy link
Member

SimenB commented Oct 12, 2020

Squash the commits? At least the some thing (feel free to remove the co-author thing)

@ttshivers ttshivers force-pushed the single_test_action branch 2 times, most recently from 02eeb6c to 92d7b18 Compare October 12, 2020 16:02
@ttshivers
Copy link
Member Author

The errors seem to be due to the flaky gpg servers. Should a time limit be put on tests to prevent it hanging from 6 hours (default max job time) if it fails?

@SimenB
Copy link
Member

SimenB commented Oct 12, 2020

As long as we don't build node it should just be a few minutes. Probably a good idea to lower the limit, yeah

@nschonni
Copy link
Member

I would thing 30minutes to 1 hour like the old Travis setup would be fine

Add a GitHub action that dynamically generates the matrix of Dockerfiles
to test based on what files have changed. This avoids having to have
generated workflows for each version x variant combo.

It looks for any changed Dockerfiles, or docker-entrypoint.sh. If any of
the testing files are changed, it will test all Dockerfiles
@ttshivers
Copy link
Member Author

Added a 60 minute timeout for each matrix entry.

@SimenB SimenB merged commit 0e87209 into nodejs:master Oct 13, 2020
@ttshivers ttshivers deleted the single_test_action branch October 13, 2020 18:59
MasonM added a commit to MasonM/docker-node that referenced this pull request Oct 23, 2021
BATS was initially added to this repository in nodejs#802, but was then
removed in nodejs#1339. This adds it back, and hooks it up to Github Actions.
MasonM added a commit to MasonM/docker-node that referenced this pull request Oct 23, 2021
BATS was initially added to this repository in nodejs#802, but was then
removed in nodejs#1339. This adds it back, and hooks it up to Github Actions.

This also fixes nodejs#1583, which happened due to a bug in the "Build image"
step: the build context was set to the root project directory, which
meant the `COPY docker-entrypoint.sh /usr/local/bin/` instruction was
copying the base `docker-entrypoint.sh` file into the Docker image
instead of the one in the variant directory. Changing the context to the
variant directory solves that.
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.

3 participants