Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add test of dev container #13
Add test of dev container #13
Changes from 26 commits
7f0df2a
7af340c
4e47a26
e82d27f
cb4f241
45f8788
11be6ca
7b45371
339bca7
313353c
3cc0dc0
d884920
de2d467
9557043
275e04b
596189f
b692acf
9fd4e1e
8280ee1
8a93100
fe2b23e
b217131
35b169d
3d95ba6
1b64daa
69e9483
07dd6f6
0b37773
07f66f8
f7426e2
4153dce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Checking out and running opensafely/research-template seems like unnecessarily tight coupling to me. What are the advantages over, say, checking in a simple
dataset_definition.py
to this repo and callingopensafely exec
?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.
The goal of this test is to check end-to-end that the dev container setup in
research-template
runs (which is a little indirect, I agree).However, it was suggested originally to keep this tests here rather than in the
research-template
repository.The reason to checkout the repository is so that we build a dev container from the
.devcontainer
configuration there.That said, I'm not sure if it's necessary to run the full research-template project (which is also tested when on PRs to that repository).
The only real extra pieces of installation that happen in the research-template
.devcontainer
configuration are:So maybe it's sufficient to check those are installed correctly (by running and importing respectively).
(It's also possible that those could move in future to here.)
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 open to this being moved or restructured, but maybe that's the subject of a subsequent PR.
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.
If the image didn't exist, then wouldn't Docker tell us? What's the benefit of this check?
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.
If you do
docker run
and the image doesn't exist, Docker will tell you.However, Docker will then immediately try and pull an arbitrary
research-template
image from the internet, which isn't at present registered, but someone could do so. There is no way that Docker gives you to disable this.I dislike this behaviour a lot. So this is a preventative measure to ensure that we only run a local image.
The alternative would be to refer to a local image with a name that we do control like
ghcr.io/opensafely-core/research-template
and give it adev
version.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.
As elsewhere, I'm not too tied to this specifically, but I'm inclined to merge this now as it's been open a while, and get the benefit of having the tests running.
If we decide this could work better, this is OK, and would be a reasonable follow-up PR.
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.
Thanks for clarifying. That is, indeed, very dislikable behaviour.