-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add check to make sure fork exists for autobump and if not create the fork #20428
Conversation
Hi @mpherman2. Thanks for your PR. I'm waiting for a kubernetes 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. |
/hold |
@@ -286,6 +286,17 @@ func Run(o *Options) error { | |||
} | |||
} | |||
|
|||
// Check to see if the proper fork exists and if it does not, create one. | |||
// TODO(mpherman): Handle case where account has repo of same name as one we are trying to autobump but is not a fork of it (e.g. test-infra) | |||
_, err := gc.GetRepo(o.GitHubLogin, o.GitHubRepo) |
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.
Can we do better error checking to differentiate from repo not exist vs. random errors? If the error type is not easy to get, maybe try use ListRepo then filter out the names(need to be careful of pagination)?
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.
Done. I originally tried to get just the error code (404) but that isn't piped to the response as far as I can tell, but I got it working with the error string instead.
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.
overall looks good to me
/lgtm
/hold
if err != nil { | ||
if err.Error() == getRepo404ErrMsg { | ||
logrus.Infof("fork needed for autobump does not exist, creating new fork of %s/%s for user %s.", o.GitHubOrg, o.GitHubRepo, o.GitHubLogin) | ||
_, err := gc.CreateFork(o.GitHubOrg, o.GitHubRepo) |
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, you can combine this line with the line below
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.
Done
/cc @alvaroaleman |
@@ -286,6 +287,21 @@ func Run(o *Options) error { | |||
} | |||
} | |||
|
|||
// Check to see if the proper fork exists and if it does not, create one. |
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.
This doesn't consider the fact that a fork needs some time to be ready after being created. Also, we already have code for this problem, can we somehow re-use the logic of one for the other?
fork := s.botUser.Login + "/" + repo |
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.
Done. I move most of the ensureFork logic to the github client so both cherrypicker and autobumper could use it. PTAL
// TODO(mpherman): Handle case where account has repo of same name as one we are trying to autobump but is not a fork of it (e.g. test-infra) | ||
_, err := gc.GetRepo(o.GitHubLogin, o.GitHubRepo) | ||
if err != nil { | ||
if err.Error() == getRepo404ErrMsg { |
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.
Please write left-aligned code to avoid hard to read passages like this, i.E. here:
if err.Error() != getRepo404ErrMsg {
return fmt.Errorf("failed to check")
}
// create fork, no need for an else clause
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.
Thanks for the link, that's pretty useful to learn
/ok-to-test |
@@ -52,6 +52,7 @@ const ( | |||
|
|||
errOncallMsgTempl = "An error occurred while finding an assignee: `%s`.\nFalling back to Blunderbuss." | |||
noOncallMsg = "Nobody is currently oncall, so falling back to Blunderbuss." | |||
getRepo404ErrMsg = "status code 404 not one of [200], body: {\"message\":\"Not Found\",\"documentation_url\":\"https://docs.github.com/rest/reference/repos#get-a-repository\"}" |
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.
This is unused now
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.
Done
prow/github/client.go
Outdated
// Fork repo if it doesn't exist. | ||
fork := forkingUser + "/" + repo | ||
repos, err := c.GetRepos(forkingUser, true) | ||
logrus.Info(repos) |
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.
Debugging remainder?
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.
Yes! Good catch. Removed
@@ -3561,6 +3562,70 @@ func (c *client) CreateFork(owner, repo string) (string, error) { | |||
return resp.Name, err | |||
} | |||
|
|||
// EnsureFork checks to see that there is a fork of org/repo in the forkedUsers repositories. | |||
// If there is not, it makes one, and waits for the fork to be created before returning. | |||
func (c *client) EnsureFork(forkingUser, org, repo string) (string, error) { |
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: Could you godoc that the returned string is the repos name and that it might be different from the original name due to a naming conflict?
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.
Done
} | ||
return false | ||
// fork repo if it doesn't exsit | ||
s.ghc.EnsureFork(s.botUser.Login, org, repo) |
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.
Can we handle the error, please?
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.
Done.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, chaodaiG, mpherman2 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 |
/unhold |
@chaodaiG @mpherman2 every single time that there's a |
@@ -286,6 +286,13 @@ func Run(o *Options) error { | |||
} | |||
} | |||
|
|||
// Check to see if the proper fork exists and if it does not, create one. | |||
// TODO(mpherman): Handle case where account has repo of same name as one we are trying to autobump but is not a fork of it (e.g. test-infra) | |||
_, err := gc.EnsureFork(o.GitHubLogin, o.GitHubOrg, o.GitHubRepo) |
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.
This cannot be ignored, you must keep track of what this is called if you're going to interact with it.
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.
Handling the situation where the fork name was different from the repo name was handled in a later PR.
return fmt.Errorf("timed out waiting for %s to appear on GitHub%s", owner+"/"+name, ghErr) | ||
} | ||
// fork repo if it doesn't exsit | ||
if _, err := s.ghc.EnsureFork(s.botUser.Login, org, repo); err != nil { |
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.
This cannot be ignored, this change breaks cherry-picker in cases where the fork is not named the same as the repo.
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.
#20826 Thanks for pointing this out.
In order for the autobumper to work, the robot doing the autobump must have a fork of the repo on their account. This checks to make sure they do, and if they do not it creates one.