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

Docker: prepare export directory #212

Merged
merged 3 commits into from May 3, 2017

Conversation

Projects
None yet
3 participants
@Strubbl
Contributor

Strubbl commented Apr 8, 2017

This commits prepares an /export directory to be used for the paperless exporter

@danielquinn

This comment has been minimized.

Owner

danielquinn commented Apr 29, 2017

This looks ok to me, but my Docker-foo is still not very good. @pitkley what do you think?

@pitkley

Good idea, small change required 👍

Dockerfile Outdated
# Set export directory
ENV PAPERLESS_EXPORT_DIR /export
RUN mkdir -p $PAPERLESS_EXPORT_DIR
RUN chown -Rh paperless:paperless $PAPERLESS_EXPORT_DIR

This comment has been minimized.

@pitkley

pitkley Apr 29, 2017

Contributor

Running this line during the build will not reliably work if the end-user host-mounts the export directory, this needs to be done in the docker-entrypoint.sh script.

You can pretty much duplicate/copy-paste the logic from the consumption directory.

@Strubbl

This comment has been minimized.

Contributor

Strubbl commented May 2, 2017

Thanks for the suggestion @pitkley . It would be nice if you can review my recent commit, too. I did not want to duplicate the code for the logic from the consumption directory, so i built a for loop.

@pitkley

Good idea with the loop. One small nit, two optional suggestions ☺️!

# Set permissions for consumption and export directory
for i in PAPERLESS_CONSUMPTION_DIR PAPERLESS_EXPORT_DIR; do
cur_dir_name=$(sed -e's/PAPERLESS_//; s/_DIR//' <<< $i | tr '[:upper:]' '[:lower:]')

This comment has been minimized.

@pitkley

pitkley May 2, 2017

Contributor

While I wouldn't mind the message being "Changing group of PAPERLESS_CONSUMPTION_DIR", I appreciate the effort to get a normal sentence.

Three things (one I see as required, the other two optional):

  1. Required: Please add a comment before the variable explaining what this does -- it took me a hot minute to understand what is going on. Maybe something like:

    # Extract the name of the current directory for the error message
    
  2. Optional: I would change the loop-variable from $i to something more speaking, e.g. $dir. $i is usually used for integer variables.

  3. Optional: I would favour using awk instead of both sed and tr, mainly because I think it enhances readability:

    cur_dir_name=$(echo "$i" | awk -F'_' '{ print tolower($2); }')
    

    But this is probably subjective, and with the added comment mentioned in (1), feel free to keep it as is! 👍

@Strubbl

This comment has been minimized.

Contributor

Strubbl commented May 2, 2017

Thanks for your feedback. I implemented all three suggestions.
I am more familiar with sed, but your awk solution is much more readable.

@pitkley

pitkley approved these changes May 2, 2017

Looks good to me, approved from my side. @danielquinn can you take a look?

@danielquinn danielquinn merged commit 85fcb5f into danielquinn:master May 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danielquinn

This comment has been minimized.

Owner

danielquinn commented May 3, 2017

Great work on this guys, thanks so much!

danielquinn added a commit that referenced this pull request May 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment