git-init update and fixes #596
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vdemeester If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -87,16 +91,18 @@ func main() { | |||
| run(logger, "git", "init") | |||
There was a problem hiding this comment.
It seems that the second git init is not needed?
And perhaps check if path+"./git" exist already before doing the git init ?
There was a problem hiding this comment.
@chmouel it's not on the same "branch", so still needed
|
Perhaps add this review too tektoncd/pipeline#747 |
|
I think |
| if err != nil { | ||
| trimedURL := strings.TrimSpace(*url) | ||
| runOrFail(logger, "git", "remote", "add", "origin", trimedURL) | ||
| if err = run(logger, "git", "fetch", "--depth=1", "--recurse-submodules=yes", "origin", *revision); err != nil { |
There was a problem hiding this comment.
Could we consider using context to handler command hangs? Today we encountered a situation which the command was stuck, and there was a problem with the underlying storage.
There was a problem hiding this comment.
@zouyee we may, with a default timeout, question is then, what would be a "sane" timeout ?
- check for errors or at least log them trim spaces from url : if the - given url contains some spaces (like non-breaking spaces), `git-init` may fail with `protocol ' https' is not supported`. This fixes that. Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
git-init-err-and-trim
branch
from
fb415d0 to
747e1cb
Compare
|
@vdemeester: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Closing this as stale, please reopen if you think this is still something we should do. |
Proposed Changes
git-initmay fail withprotocol ' https' is not supported. Thisfixes that.
Related to tektoncd/pipeline#677 and tektoncd/pipeline#750
Signed-off-by: Vincent Demeester [email protected]