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
Move root-required bits of drama-free-django build into Dockerfile #5145
Move root-required bits of drama-free-django build into Dockerfile #5145
Changes from 14 commits
fb34088
f2269c6
8478002
270e1a6
a9a63bb
a6e09f5
7431fba
490fe09
3cd92f0
139f8d8
e936ae7
4433633
44fef31
5b963b4
8bf713f
72e7382
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.
Not a big deal, but I'm curious why you made a new directory for this. I figured that working in /tmp was fine since the container is ephemeral.
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 issue was that by having it in
/tmp
, it resulted in directories being created in the/
root directory, and the non-root
user didn't have permission to create those directories. An alternative would be to open up permissions on/
, but this seems like a better option.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.
This shouldn't hold up this PR, but, if we are going to maintain this DFD testing capability, I wonder if it's worth entertaining the idea of a distinct Dockerfile just for that purpose. We don't really need everything in the "cfgov-dfd-builder" image just to run the DFD image, and it would be nice to actually determine what is needed. But probably thinking more about that should wait until/if we want to think about migrating Ansible code 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.
Yeah, it does feel a little awkward to have the two scripts that both build the same image, but my assumption was that generally when you'd run
test.sh
, you would have runbuild.sh
just before it, and so thedocker build
part would be fully cached and be pretty instant, but I didn't want to make that a requirement to runningtest.sh
, so it's duplicated in both scripts.As for a separate image, yeah we could. It just seemed like it'd be better to only maintain one Dockerfile that could do both. But if there's a scenario where we'd be running just
test.sh
, maybe that starts to make more sense.