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
update cloneOrOpenRepo to verify matching remote before updating repo. #308
base: main
Are you sure you want to change the base?
update cloneOrOpenRepo to verify matching remote before updating repo. #308
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DannyBrito The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @DannyBrito! |
Hi @DannyBrito. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
322b3e5
to
7ffc6d8
Compare
/retest |
Thank you for the contribution @DannyBrito! This verification will work fine for repos cloned using our tooling, however, we might run into issues with repos cloned manually or using other tooling (which is supported by the
We could try to do some pattern matching, but that still sounds kind of risky in my opinion. Maybe we should consider if we can fix this directly in the |
@DannyBrito do you want to go ahead and update |
r, err := OpenRepo(repoPath) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if updateRepository { | ||
remotes, err := r.Remotes() | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to get repository remotes: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
return nil, fmt.Errorf("unable to get repository remotes: %v", err) | |
return nil, fmt.Errorf("unable to get repository remotes: %w", err) |
With that we break one beneficial use case: Being able to run the tool multiple times (without specifying the repo path) on a single repo while only pulling on the first run. For k/k this was particularly helpful because the repo is huge and pulling takes quite some time. |
@saschagrunert What about instead of completely randomizing the directory name, we build it based on org and repo, e.g. |
@xmudrii that would be great and I think it will work pretty well! |
I think @xmudrii 's suggestion would be great. @DannyBrito do you want to try that out? |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
What type of PR is this?
What this PR does / why we need it:
This PR adds verification of matching remote within repo to be updated/clone in function
updateRepo
If repo URL from the repo to be clone or updated matches any remote in repo will proceed as normal, otherwise will return a new
ErrNoMatchingRemote
errorThis will be helpful for better handling, in scenario where the given
repoPath
exists but this doesn't have a remote pointing to target repo to clone or update. This issue can be seen in kubernetes/release#3444cc: @jeremyrickard
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?