Skip to content

ci: use single dynamic build-test action#1339

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.

Sign up for GitHub

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

Jump to bottom
Merged
SimenB merged 1 commit into from
Oct 13, 2020
Merged

ci: use single dynamic build-test action #1339

SimenB merged 1 commit into from
Oct 13, 2020

Conversation

@ttshivers
Copy link
Member

@ttshiversttshivers commented Sep 27, 2020

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

nschonni
nschonni reviewed Sep 27, 2020
View reviewed changes
Copy link
Member

@nschonninschonni 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

@ttshivers
Copy link
MemberAuthor

ttshivers commented Sep 27, 2020

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

nschonni commented Sep 27, 2020

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 😆

nschonni
nschonni reviewed Sep 27, 2020
View reviewed changes
Copy link
Member

@nschonninschonni 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

@ttshivers
Copy link
MemberAuthor

ttshivers commented Sep 28, 2020•
edited
Loading

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

nschonni 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.

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

SimenB
SimenB approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@SimenBSimenB 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

@SimenBSimenB requested a review from a teamSeptember 28, 2020 08:21
@ttshiversttshiversforce-pushed the single_test_action branch 2 times, most recently from 6c8543e to aa615beCompareSeptember 28, 2020 19:24
nschonni
nschonni reviewed Sep 28, 2020
View reviewed changes
SimenB
SimenB reviewed Oct 1, 2020
View reviewed changes
@SimenBSimenB requested review from a team and nschonniOctober 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
MemberAuthor

ttshivers 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? 🙂

Deleted those files

@SimenB
Copy link
Member

SimenB commented Oct 12, 2020•
edited
Loading

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

@ttshiversttshiversforce-pushed the single_test_action branch 2 times, most recently from 02eeb6c to 92d7b18CompareOctober 12, 2020 16:02
@ttshivers
Copy link
MemberAuthor

ttshivers commented Oct 12, 2020

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

nschonni commented Oct 12, 2020

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

@ttshivers
ci: use single dynamic build-test action
ee169b2
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
MemberAuthor

ttshivers commented Oct 12, 2020

Added a 60 minute timeout for each matrix entry.

@SimenBSimenB merged commit 0e87209 into nodejs:masterOct 13, 2020
@SimenBSimenB mentioned this pull request Oct 13, 2020
Closed
@ttshiversttshivers deleted the single_test_action branch October 13, 2020 18:59
This was referenced Oct 20, 2021
Merged
Open
MasonM added a commit to MasonM/docker-node that referenced this pull request Oct 23, 2021
@MasonM
feat: Add back BATS for testing images
441dfbe
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
@MasonM
feat: Add back BATS for testing images
f8853c4
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 fixesnodejs#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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SimenBSimenBSimenB approved these changes

@nschonninschonninschonni approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ttshivers@nschonni@SimenB