Skip to content
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

Dockerfile Tweaks and Cleanup #143

Merged
merged 3 commits into from
Dec 1, 2017
Merged

Dockerfile Tweaks and Cleanup #143

merged 3 commits into from
Dec 1, 2017

Conversation

egyptiankarim
Copy link
Contributor

@egyptiankarim egyptiankarim commented Nov 30, 2017

Addressing issue #144:

  • I corrected the variable expansion issue with WORKDIR in the Dockerfile.
  • Did some light "copy editing".
  • Deleted the entrypoint.sh script.
  • Corrected some of the flags in run.

Addressing issue #:

- I corrected the variable expansion issue with `WORKDIR` in the `Dockerfile`.
- Did some light "copy editing".
- Deleted the `entrypoint.sh` script.
- Corrected some of the flags in `run`.
@egyptiankarim egyptiankarim mentioned this pull request Nov 30, 2017
@konklone
Copy link
Collaborator

I love deleting code! And it looks good to me, but deferring to @jsf9k as a better Docker reviewer.

@jsf9k jsf9k self-assigned this Nov 30, 2017
@jsf9k
Copy link
Member

jsf9k commented Nov 30, 2017

@egyptiankarim, thanks for catching the error and thanks for getting rid of entrypoint.sh!

@jsf9k jsf9k self-requested a review November 30, 2017 23:01
Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested one very minor change. After that I'll approve and merge it. (I'd make the one-line change myself, but I don't seem to have permission push to this branch.)

Dockerfile Outdated
RUN mkdir ${PSHTT_HOME}

# Create an unprivileged user
RUN groupadd --system pshtt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egyptiankarim, do you mind combining these two RUN statements like they were originally? I'm kind of insane about combining related statements where possible to reduce the number of layers in the Docker image.


# Install pshtt
COPY . ${PSHTT_HOME}
RUN chown -R pshtt:pshtt ${PSHTT_HOME}
RUN pip install --no-cache ${PSHTT_HOME}

# Prepare to run
WORKDIR PSHTT_HOME
WORKDIR ${PSHTT_HOME}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

run
`# Change the ownership of the files (**e.g** results) to the user with id 1042 and to the group with id 1042` \
-e USER_ID=1042 \
-e GROUP_ID=1042 \
-v $(pwd):/home/pshtt \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting rid of some magic numbers! 👍

@konklone
Copy link
Collaborator

@egyptiankarim You should be able to check a box somewhere that lets @jsf9k make changes on the PR directly. I know it lets you do at post-time, not as certain about after posting.

@egyptiankarim
Copy link
Contributor Author

egyptiankarim commented Dec 1, 2017

@konklone @jsf9k it's strange because I already have the little "allow edits from maintainers" box checked. Hopefully the latest commit addresses the requested change. Let me know if I need to make any more 😊

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@jsf9k jsf9k merged commit 4e9505d into cisagov:master Dec 1, 2017
cisagovbot pushed a commit that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants