-
Notifications
You must be signed in to change notification settings - Fork 2k
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: Add back BATS for testing images #1588
base: main
Are you sure you want to change the base?
Conversation
01c1bce
to
67017f6
Compare
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.
|
||
- name: Test for npm | ||
run: docker run --rm node:${{ matrix.version }}-${{ matrix.variant }} npm --version | ||
context: ./${{ steps.short-version.outputs.result }}/${{ matrix.variant }} |
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.
Here's the fix for issue #1583, in case it wasn't clear from the PR description. The context should be the variant directory so it copies the right docker-entrypoint.sh
file.
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 think this makes sense, thanks!
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 not sure I see the value of adding back BATS for these tests. If we do add it back, I think using the old install apt-install method would be better than a submodule.
I think added your other tests the same way as the others currently are there would make sense though
Unfortunately, the version of "bats" in Ubuntu 20.04 is quite old and lacks support for the I guess I could have Github Actions download the BATS source with But that's more complicated than using a submodule. |
I think using a submodule makes sense since that's what the official docs recommend: https://bats-core.readthedocs.io/en/stable/tutorial.html#quick-installation. I know I suggested it in #1588 (comment), but since it has no dependencies we could also just run it through |
@SimenB Okay, I removed the submodule and changed it to use |
Is this still pending another review? |
I would like one 😀 |
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.
Copilot reviewed 1 out of 2 changed files in this pull request and generated no suggestions.
Files not reviewed (1)
- test/docker-image.bats: Language not supported
Description
BATS was initially added to this repository in #802, but was then removed in #1339. This adds it back, and hooks it up to Github Actions.
This also fixes #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 basedocker-entrypoint.sh
file into the Docker image instead of the one in the variant directory. Changing the context to the variant directory solves that.Motivation and Context
#1579 (comment)
Testing Details
Link to run for this PR: https://github.com/nodejs/docker-node/actions/runs/1374569169
Example Output(if appropriate)
N/A
Types of changes
Checklist