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

Empty directories from local file system not preserved. #367

Closed
GrahamDumpleton opened this issue Nov 25, 2015 · 8 comments
Closed

Empty directories from local file system not preserved. #367

GrahamDumpleton opened this issue Nov 25, 2015 · 8 comments
Assignees
Labels
area/usability kind/bug Categorizes issue or PR as related to a bug. priority/P3

Comments

@GrahamDumpleton
Copy link

As mentioned in #348, when S2I is used against a git repo it doesn't have to worry about empty directories because to have an empty directory in git you need to add a dummy file such as .gitkeep to it also git will never add the empty directory to the report.

When running S2I against the file system though, an empty directory could well exist as no requirement to have a dummy file in it. Unfortunately the directory when empty isn't preserved by S2I and it never created in the image. This means that if any code expected the empty directory to exist when run in the image, it will fail.

@mfojtik mfojtik added area/usability kind/bug Categorizes issue or PR as related to a bug. labels Dec 15, 2015
@gabemontero
Copy link
Contributor

So after reading this and #348 a couple of times, I suppose we could, after finding the empty directories off of the targetPath (after the cp -a sourcePath targetPath) via a find targetPath -type d -empty

  1. add a .gitkeep file to any empty directories on the targetPath slide found so git respects them
  2. explicitly document we don't support empty dirs, and remove them from the targetPath after the cp -a

I suppose 1) is better, but wonder if I'm missing (unfavorable) implications down the line.

@bparees @csrwng @mfojtik @GrahamDumpleton - any opinions / suggestions?

@mfojtik
Copy link
Contributor

mfojtik commented Mar 14, 2016

@gabemontero another option would be to add special flag to force "file" SCM, so the directory with source will be copied instead of 'git cloned'.

@gabemontero
Copy link
Contributor

ah, that's right @mfojtik , thanks for the reminder ... s2i today has a generic "scm" concept, where the 2 impls or plugins at the moment are git and file.

Before moving to next steps, a quick recap of what the code does in determining which to use:

  1. it takes the source spec and sees if the file exists
  2. it then looks to see if either a) the git binary is not in the path, or b) it looks to see if a .git file does not exists off of the root of the source spec provided

If either a) or b) are true, then it uses local copy.

So in @GrahamDumpleton 's scenario, if it ended up doing a git clone where a local dir path was provided, either the path did not in fact exist, or the top dir had a .git file ... i.e. the source dir tree was originally created by a git clone, and then empty directories were created after the fact.

Certainly seems like a reasonable dev path, and expecting the user to remove .git files and force the copy behavior based on how our impl is currently done is a bit much.

I'll start down the add the config option path, unless the typical concerns about adding another config option are too high in this case vs. the value of satisfying this scenario.

@bparees
Copy link
Contributor

bparees commented Mar 14, 2016

I think i'm ok w/ the "forceCopy" (vs clone) flag to address this.

@gabemontero
Copy link
Contributor

Is that --forceCopy, with a default of false (i.e. stay consistent with the existing behavior), with perhaps also a -c, or just the --forceCopy ?

@gabemontero
Copy link
Contributor

though the other options with s2i are hyphen separated ... assemble-user, context-dir, scripts-url, use-config ... so probably should be force-copy

@mfojtik
Copy link
Contributor

mfojtik commented Mar 14, 2016

--force-copy or just --copy would be OK i think

@bparees
Copy link
Contributor

bparees commented Mar 14, 2016

if "-c" is available, sure let's include that shorthand. --copy sounds ok for the long-form flag name.

and yes, default to false(existing behavior).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/usability kind/bug Categorizes issue or PR as related to a bug. priority/P3
Projects
None yet
Development

No branches or pull requests

4 participants