Allow user to specify git clone directory in gitRepo volume #15533
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
Labelling this PR as size/M |
k8s-github-robot
added
needs-rebase
|
@kubernetes/rh-cluster-infra @kubernetes/rh-storage @kubernetes/rh-dev-experience @kubernetes/rh-ux |
| } else { | ||
| if output, err := b.execCommand("git", []string{"clone", b.source}, dir); err != nil { | ||
| return fmt.Errorf("failed to exec 'git clone %s': %s: %v", b.source, output, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
More concise this way and also supports N git args in the error message:
args := []string{"clone", b.source}
if b.target != "" {
args = append(args, b.target)
}
if output, err := b.execCommand("git", args, dir); err != nil {
return fmt.Errorf("failed to exec 'git %s': %s: %v", strings.Join(append(args[:1], args[1:len(args)]...), " "), output, err)
}
There was a problem hiding this comment.
Will this part fail if you specify a target? The current implementation assumes there will be exactly 1 directory (the repo name) in the volume.
if len(files) != 1 {
return fmt.Errorf("unexpected directory contents: %v", files)
}
I couldn't comment on that line in the diff because you didn't edit it.
There was a problem hiding this comment.
This will fail when clone directory is '.'
Actually, see my comments here: #15421 (comment)
I'd like to update the code if we reached a agreement.
There was a problem hiding this comment.
As per #15421
How urgent is this one small feature? I'm not deeply against adding it if
it solves real problems, but I'd rather focus energy on the git-sync
sidecar. Additionally, we need clarity on whether this is just "checkout
and rename" or whether "." is expected to be supported. Doing a checkout to
a subdir has one significant advantage - it can be updated atomically. This
means you could periodically sync to head of a branch, which seems like a
powerful feature to me.
If we support ".", that does not work.
On Wed, Oct 14, 2015 at 12:54 AM, Harry Zhang notifications@github.com
wrote:
In pkg/volume/git_repo/git_repo.go
#15533 (comment)
:@@ -138,8 +140,15 @@ func (b *gitRepoVolumeBuilder) SetUpAt(dir string) error {
return err
}
- if output, err := b.execCommand("git", []string{"clone", b.source}, dir); err != nil {
return fmt.Errorf("failed to exec 'git clone %s': %s: %v", b.source, output, err)- // If provided target directory
- if "" != b.target {
if output, err := b.execCommand("git", []string{"clone", b.source, b.target}, dir); err != nil {return fmt.Errorf("failed to exec 'git clone %s %s': %s: %v", b.source, b.target, output, err)}- } else {
if output, err := b.execCommand("git", []string{"clone", b.source}, dir); err != nil {return fmt.Errorf("failed to exec 'git clone %s': %s: %v", b.source, output, err)}This will fail when clone directory is '.'
Actually, see my comments here: #15421 (comment)
#15421 (comment)I'd like to update the code if we reached a agreement.
—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/15533/files#r41964131.
resouer
force-pushed
the
git-volume
branch
3 times, most recently
from
b86afb4
to
4ae9844
Compare
|
@markturansky Although discusss is in progess in original issue, I updated this with tests |
resouer
force-pushed
the
git-volume
branch
2 times, most recently
from
5ebab60
to
e70b708
Compare
k8s-github-robot
added
size/XXL
|
Labelling this PR as size/XXL |
|
|
| return fmt.Errorf("unexpected directory contents: %v", files) | ||
| } | ||
| subdir = path.Join(dir, files[0].Name()) | ||
| } |
There was a problem hiding this comment.
Better to use switch over if/else:
switch {
case b.target == "":
case b.target != "" && len(files) == 1:
case b.target != && len(files) != 1
}
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
| if len(files) != 1 { | ||
| return fmt.Errorf("unexpected directory contents: %v", files) | ||
| } | ||
|
|
||
| if len(b.revision) == 0 { |
There was a problem hiding this comment.
len(b.revision) == 0 and b.target != "" are the same (in effect) but different implementations. They should be consistent. I've mostly seen (and used) empty string "" comparison instead of string length check.
|
What kind of validation is required for the target directory? "." and a path are valid. Is "../path" valid? numbers/spaces/special characters? What's the worst someone can put into the target string? |
|
@markturansky I don't think numbers/special characters are harmful, at firs glance.
Or, we can only allow: [a-z][0-9][.], and refuse others. I prefer this way in this PR. |
|
Labelling this PR as size/XS |
k8s-github-robot
added
size/XS
|
GCE e2e test build/test passed for commit d051cc1de6ab715ea7e8a00b36c26a3ddeb4ffc9. |
|
GCE e2e test build/test passed for commit d65fedc29537b502671115658a25e8204eb2246a. |
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
the
needs-rebase
|
Labelling this PR as size/L |
k8s-github-robot
added
size/L
|
GCE e2e test build/test passed for commit e6ff227aa79ebea18082ace79b473489c9bfb98c. |
Update validate and gitRepo Update generated code
pmorie
added
lgtm
|
Continuous integration appears to have missed, closing and re-opening to trigger it |
|
GCE e2e test build/test passed for commit 70a9c0b. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 70a9c0b. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
Fix #15421