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 Dockerfile to enable running end-to-end tests with docker #1230
Add Dockerfile to enable running end-to-end tests with docker #1230
Changes from 20 commits
2327bf5
883b384
357c5f9
c41193e
7112159
b10cf5d
882a349
0e4f54c
e7f54e2
9f91cfc
75b14c7
4a5cb90
dec2ddf
ba25373
cf9ebec
98bd231
244afe7
3396883
108ece8
88e3d1c
d7d2213
50f8fcc
4735044
86c74b8
6071758
4895ecb
9cbb991
d45156f
59e9c47
bb31f84
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.
Really shouldn't be necessary to rely on sudo if the intent is to replace Vagrantfile with Dockerfile.
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 agree, I just don't want to touch all the internal files because a) I'm not too familiar with them and b) I'm not sure it'll behave the same as I couldn't run the Vagrant tests. I think we should go iteratively on this, by first making it compatible and afterwards refactoring the
sudo
stuffThere 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.
Ignore this
This also has some odd locale logic at the end. I'm wondering if that's necessary?
In
docker-mailserver
image we have a similar locale section:RUN <<EOF rm -rf /usr/share/locale/* rm -rf /usr/share/man/* rm -rf /usr/share/doc/* update-locale EOF
For exa, I tracked it down to a specific test for different locales, so ignore this feedback 😅
115315a
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 if you can use
mknod
with the image build to get the/dev
special files created:exa/devtools/dev-create-test-filesystem.sh
Lines 126 to 128 in f039202
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.
Sorry, I don't understand. Do you mean that
mknod
won't work in docker?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.
Yeah, I don't think it works, but I could be mistaken, I haven't checked 🤷♂️
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.
kconv should be located down here with the rest? I assume the concern was due to the package install portion for kconv and moving the layer(s) down to the end here being problematic when an earlier instruction invalidates the
Dockerfile
cache?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.
You could actually shift these up to after kconv, shouldn't be an issue given what the rest of the stage is doing?
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.
Actually it's needed for
RUN cargo kcov --print-install-kcov-sh | sh
hereThat's true, do you think it's useful? I don't understand the benefit
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 layers that are more frequently to change should come later. The bulk of this stage is something that could run in a separate script via
RUN
, but for now you've stated you want to keep it in theDockerfile
.You could move these
COPY
lines up beforekconv
for better co-location, but right now no major concern either way.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.