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

Allow REPO_DIR to be a non-existing folder by creating it and providing the user with permissions #976

Merged
merged 4 commits into from Oct 31, 2022

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Oct 19, 2020

PR summary by Erik 2022-10-31

  • Non-existing REPO_DIR folder
    If users provide a REPO_DIR as a build arg to a folder that doesn't exist, the root user will create it but leave the non-root user without permissions to access it. This PR resolves that and adds a test to verify it by adding some mkdir and chown statements.
  • Dockerfile refactoring
    It includes basic refactoring of the Dockerfile to reduce the layers by coalescing ENV statements and making use of the recommended syntax with explicit = in ENV statements.
  • Refined test documentation
    It updates a comment to clarify how the test suite works.

Updated

This PR (originally attempted to try out some alternatives to #974 and (#975) now:

  • changes the target dir TBD (might not, if [MRG] Ensure REPO_DIR owned by NB_USER #975 changes are sufficient: @manics and @tomyun offered some additional approaches to be considered/discussed)

  • adds some additional read-/write-ability checks to the custom target dir test case
    • in the process of figuring out where to land it...
      • expands the docs in conftest.py for providing extra arguments via extra-args.yml
  • partially to offset layer addition for path checking, where possible...
    • updates all uses of ENV to the recommended = syntax, replacing use of the "alternative" (space-based) syntax:

      The alternative syntax is supported for backward compatibility, but discouraged for the reasons outlined above, and may be removed in a future release.

    • combines multiple consecutive ENV calls to a single, multi-line directive

original

Some guidance on where to test this would be helpful... looking through the examples, I didn't see a way to add cli args, e.g. a repo2docker-cli-args file, perhaps that would be useful? this could be piggybacked off any of the existing ones, then...

I found ☝️ and added some docs in conftest.py. Since the conda example was already testing repo-dir, i just added the test in that place.

RUN if [ ! -d "${REPO_DIR}" ] \
; then \
mkdir -p "${REPO_DIR}" \
&& chown -R ${NB_USER}:${NB_USER} "${REPO_DIR}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can remove -R as it's an empty directory just created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it probably doesn't magically do umask or anything (though maybe we should think about that)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will wait to get any other feedback, it's a lot of coal to burn!

Copy link
Member

Choose a reason for hiding this comment

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

FYI you can use something like /usr/bin/install -d <directory> -m <mode> -o <owner> -g <group> to do everything in one go if you want, doesn't really matter though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, can we use this instead? if in bash always makes me feel like I'm too dumb to make it work

Copy link
Member

Choose a reason for hiding this comment

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

This PR as a whole LGTM, but let's adjust to this review comment and resolve the merge conflicts and we are good to go.

Copy link
Member

Choose a reason for hiding this comment

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

Resolved it by applying my own suggestion, based on /usr/bin/install -d <directory> -m <mode> -o <owner> -g <group>

@bollwyvl bollwyvl changed the title ensure existence/permissions on custom target dir [wip] combine/normalize ENV syntax, update test docs Oct 20, 2020
@manics manics marked this pull request as draft January 26, 2022 19:22
@consideRatio consideRatio changed the title [wip] combine/normalize ENV syntax, update test docs combine/normalize ENV syntax, update test docs Oct 30, 2022
@consideRatio consideRatio changed the title combine/normalize ENV syntax, update test docs Add permissions for non-existent REPO_DIR folders Oct 31, 2022
@consideRatio consideRatio changed the title Add permissions for non-existent REPO_DIR folders Allow REPO_DIR to be a non-existing folder by creating it and providing the user with permissions Oct 31, 2022
@consideRatio consideRatio marked this pull request as ready for review October 31, 2022 10:04
@consideRatio
Copy link
Member

consideRatio commented Oct 31, 2022

I applied a suggestion and rebased this PR on the main branch to get the test suite to run.

I took a backup of the state before rebase in my fork's branch.

@consideRatio
Copy link
Member

Tests pass, I'll make another rebase to get a bit cleaner git history before merge.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPO_DIR permission denied
6 participants