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

fix(resolver::git): refine repos cloning and fix git checkout #1511

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

knight42
Copy link
Collaborator

@knight42 knight42 commented Oct 27, 2020

What this PR does / why we need it:

Simplify the way we clone repos, and don't pass -f to git checkout
so that git won't fail silently.

Manual validation:

branch:

Trying to acquire lock and pull resource
Got the lock, start to pulling...
Clone refs/heads/master...
Initialized empty Git repository in /workspace/data/.git/
From http://<redacted>/root/golangtest
 * branch            master     -> FETCH_HEAD
 * [new branch]      master     -> origin/master
total 1
drwxr-xr-x 1 root root   3 Oct 27 14:30 .
drwxr-xr-x 1 root root   2 Oct 27 14:30 ..
drwxr-xr-x 1 root root  12 Oct 27 14:30 .git
-rw-r--r-- 1 root root 249 Oct 27 14:30 Dockerfile
drwxr-xr-x 1 root root   2 Oct 27 14:30 helloworld
Collect commit id to result file /__result__ ...
LastCommitID:c1078d2b17aa201f26e2daf4d74437b5df2eb169
Remove the created lock folder

tag

Trying to acquire lock and pull resource
Got the lock, start to pulling...
Clone refs/tags/v0.0.1...
Initialized empty Git repository in /workspace/data/.git/
From http://<redacted>/root/golangtest
 * tag               v0.0.1     -> FETCH_HEAD
total 1
drwxr-xr-x 1 root root   3 Oct 27 14:33 .
drwxr-xr-x 1 root root   2 Oct 27 14:33 ..
drwxr-xr-x 1 root root  12 Oct 27 14:33 .git
-rw-r--r-- 1 root root 249 Oct 27 14:33 Dockerfile
drwxr-xr-x 1 root root   2 Oct 27 14:33 helloworld
Collect commit id to result file /__result__ ...
LastCommitID:c1078d2b17aa201f26e2daf4d74437b5df2eb169
Remove the created lock folder

pull request

Trying to acquire lock and pull resource
Got the lock, start to pulling...
Merge refs/merge-requests/1/head to master...
Cloning into 'data'...
POST git-upload-pack (192 bytes)
POST git-upload-pack (201 bytes)
From http://<redacted>/root/golangtest
 * branch            refs/merge-requests/1/head -> FETCH_HEAD
Automatic merge went well; stopped before committing as requested
total 1
drwxr-xr-x 1 root root   3 Oct 27 14:37 .
drwxr-xr-x 1 root root   3 Oct 27 14:37 ..
drwxr-xr-x 1 root root  16 Oct 27 14:37 .git
-rw-r--r-- 1 root root 250 Oct 27 14:37 Dockerfile
drwxr-xr-x 1 root root   2 Oct 27 14:37 helloworld
Collect commit id to result file /__result__ ...
LastCommitID:c1078d2b17aa201f26e2daf4d74437b5df2eb169
Remove the created lock folder

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes #

Reference to #

Special notes for your reviewer:

/cc @zhujian7

Release note:

NONE

simplify the way we clone repos, and don't pass `-f` to `git checkout`
so that git won't fail silently.
@caicloud-bot caicloud-bot added release-note-none Denotes a PR that doesn't merit a release note. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2020
@caicloud-bot caicloud-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2020
fi
fi
return 0
retryGitOpWithoutDepthFlagIfFromBitbucket() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add some comments for why bitbucket needs to retry without depth flag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added.

@hyy0322
Copy link
Contributor

hyy0322 commented Oct 27, 2020

Nice work. Did you test bitbucket case? @knight42

echo "Existed data not a valid git repo for ${SCM_URL##*//}"
exit 1
}

echo "Fetch $SCM_REVISION from origin"
git fetch -v ${GIT_DEPTH_OPTION:-} origin $SCM_REVISION
retryGitOpWithoutDepthFlagIfFromBitbucket fetch -v origin "$SCM_REVISION"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the function name should be renamed normally like cloneRetry blablabla, or something like that. And comment bitbucket.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMHO it is not quite convenient to view the comment of a function in bash, and I think it might be more obvious to user to use an informative(but maybe verbose) function name.

@knight42
Copy link
Collaborator Author

@hyy0322 hmm not yet, I would test with bitbucket later.

@zhujian7
Copy link
Collaborator

LGTM

@zhujian7
Copy link
Collaborator

@hyy0322 PTAL

@hyy0322
Copy link
Contributor

hyy0322 commented Oct 27, 2020

LGTM

@zhujian7
Copy link
Collaborator

/lgtm

@zhujian7
Copy link
Collaborator

/cherrypick cps-2.10-dood

@caicloud-bot
Copy link
Collaborator

@zhujian7: once the present PR merges, I will cherry-pick it on top of cps-2.10-dood in a new PR and assign it to you.

In response to this:

/cherrypick cps-2.10-dood

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.

@zhujian7
Copy link
Collaborator

/cherrypick cps-2.10

@caicloud-bot
Copy link
Collaborator

@zhujian7: once the present PR merges, I will cherry-pick it on top of cps-2.10 in a new PR and assign it to you.

In response to this:

/cherrypick cps-2.10

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.

@zhujian7
Copy link
Collaborator

/cherrypick cps-2.8

@caicloud-bot caicloud-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2020
@caicloud-bot
Copy link
Collaborator

@zhujian7: once the present PR merges, I will cherry-pick it on top of cps-2.8 in a new PR and assign it to you.

In response to this:

/cherrypick cps-2.8

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.

@zhujian7
Copy link
Collaborator

/cherrypick cps-2.11

@caicloud-bot
Copy link
Collaborator

@zhujian7: once the present PR merges, I will cherry-pick it on top of cps-2.11 in a new PR and assign it to you.

In response to this:

/cherrypick cps-2.11

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.

@caicloud-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhujian7

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@caicloud-bot caicloud-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2020
@caicloud-bot caicloud-bot merged commit 717061b into caicloud:master Oct 27, 2020
@caicloud-bot
Copy link
Collaborator

@zhujian7: new pull request created: #1513

In response to this:

/cherrypick cps-2.10-dood

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.

@caicloud-bot
Copy link
Collaborator

@zhujian7: new pull request created: #1514

In response to this:

/cherrypick cps-2.10

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.

@caicloud-bot
Copy link
Collaborator

@zhujian7: new pull request created: #1515

In response to this:

/cherrypick cps-2.8

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.

@caicloud-bot
Copy link
Collaborator

@zhujian7: #1511 failed to apply on top of branch "cps-2.11":
```Applying: fix(resolver::git): refine repos cloning and fix git checkout
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M build/resolver/git/entrypoint.sh
Falling back to patching base and 3-way merge...
Auto-merging build/resolver/git/entrypoint.sh
CONFLICT (content): Merge conflict in build/resolver/git/entrypoint.sh
Patch failed at 0001 fix(resolver::git): refine repos cloning and fix `git checkout`


<details>

In response to [this](https://github.com/caicloud/cyclone/pull/1511#issuecomment-717289032):

>/cherrypick cps-2.11


Instructions for interacting with me using PR comments are available [here](https://github.com/caicloud/engineering/blob/master/docs/caicloud_bot.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

@knight42 knight42 deleted the fix/git-resolver branch October 28, 2020 07:22
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants