Add check to make sure fork exists for autobump and if not create the fork #20428

mpherman2 · 2021-01-09T01:20:27Z

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.

k8s-ci-robot · 2021-01-09T01:20:35Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

mpherman2 · 2021-01-09T01:20:50Z

/hold

chaodaiG · 2021-01-11T15:09:02Z

experiment/autobumper/bumper/bumper.go

@@ -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)


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)?

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.

chaodaiG

overall looks good to me
/lgtm

/hold

chaodaiG · 2021-01-11T21:23:54Z

experiment/autobumper/bumper/bumper.go

+		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)


nit, you can combine this line with the line below

chaodaiG · 2021-01-11T21:25:00Z

/cc @alvaroaleman

alvaroaleman · 2021-01-11T21:35:24Z

experiment/autobumper/bumper/bumper.go

@@ -286,6 +287,21 @@ func Run(o *Options) error {
 			}
 		}

+		// Check to see if the proper fork exists and if it does not, create one.


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?

test-infra/prow/external-plugins/cherrypicker/server.go

Line 521 in 299a5a1

fork := s.botUser.Login + "/" + repo

Done. I move most of the ensureFork logic to the github client so both cherrypicker and autobumper could use it. PTAL

alvaroaleman · 2021-01-11T21:37:34Z

experiment/autobumper/bumper/bumper.go

+		// 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 {


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

Thanks for the link, that's pretty useful to learn

petr-muller · 2021-01-13T13:08:21Z

/ok-to-test

alvaroaleman · 2021-01-13T14:20:36Z

experiment/autobumper/bumper/bumper.go

@@ -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\"}"


This is unused now

alvaroaleman · 2021-01-13T14:25:36Z

prow/github/client.go

+	// Fork repo if it doesn't exist.
+	fork := forkingUser + "/" + repo
+	repos, err := c.GetRepos(forkingUser, true)
+	logrus.Info(repos)


Debugging remainder?

Yes! Good catch. Removed

alvaroaleman · 2021-01-13T14:28:12Z

prow/github/client.go

@@ -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) {


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?

alvaroaleman · 2021-01-13T14:32:23Z

prow/external-plugins/cherrypicker/server.go

-	}
-	return false
+	// fork repo if it doesn't exsit
+	s.ghc.EnsureFork(s.botUser.Login, org, repo)


Can we handle the error, please?

… fork

k8s-ci-robot · 2021-01-13T23:33:58Z

[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:

~~OWNERS~~ [chaodaiG]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chaodaiG · 2021-01-13T23:34:39Z

/unhold

stevekuznetsov · 2021-02-10T23:35:33Z

@chaodaiG @mpherman2 every single time that there's a _, err := for these calls in this PR, it's 100% wrong. I'm a bit surprised those bugs are here, since they ignore the case where the fork is of a different name from the original, which is the entire point of these changes, no?

stevekuznetsov · 2021-02-10T23:36:08Z

experiment/autobumper/bumper/bumper.go

@@ -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)


This cannot be ignored, you must keep track of what this is called if you're going to interact with it.

Handling the situation where the fork name was different from the repo name was handled in a later PR.

stevekuznetsov · 2021-02-10T23:36:31Z

prow/external-plugins/cherrypicker/server.go

-			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 {


This cannot be ignored, this change breaks cherry-picker in cases where the fork is not named the same as the repo.

#20826 Thanks for pointing this out.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 9, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 9, 2021

k8s-ci-robot requested review from chaodaiG and e-blackwelder January 9, 2021 01:20

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jan 9, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2021

chaodaiG reviewed Jan 11, 2021

View reviewed changes

mpherman2 force-pushed the add_fork branch from cabc175 to 7075735 Compare January 11, 2021 18:30

chaodaiG reviewed Jan 11, 2021

View reviewed changes

k8s-ci-robot assigned chaodaiG Jan 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021

k8s-ci-robot requested a review from alvaroaleman January 11, 2021 21:25

mpherman2 force-pushed the add_fork branch from 7075735 to 4eb616a Compare January 11, 2021 21:36

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021

alvaroaleman reviewed Jan 11, 2021

View reviewed changes

mpherman2 force-pushed the add_fork branch from 4eb616a to 0f56175 Compare January 12, 2021 21:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/prow Issues or PRs related to prow and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 12, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 13, 2021

alvaroaleman reviewed Jan 13, 2021

View reviewed changes

mpherman2 force-pushed the add_fork branch from 0f56175 to 7db4f62 Compare January 13, 2021 21:14

Add check to make sure fork exists for autobump and if not create the…

cc703a4

… fork

mpherman2 force-pushed the add_fork branch from 7db4f62 to cc703a4 Compare January 13, 2021 21:50

alvaroaleman approved these changes Jan 13, 2021

View reviewed changes

k8s-ci-robot assigned alvaroaleman Jan 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2021

chaodaiG approved these changes Jan 13, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2021

k8s-ci-robot merged commit c6d9cf6 into kubernetes:master Jan 13, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 13, 2021

stevekuznetsov reviewed Feb 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check to make sure fork exists for autobump and if not create the fork #20428

Add check to make sure fork exists for autobump and if not create the fork #20428

mpherman2 commented Jan 9, 2021

k8s-ci-robot commented Jan 9, 2021

mpherman2 commented Jan 9, 2021

chaodaiG Jan 11, 2021

mpherman2 Jan 11, 2021

chaodaiG left a comment

chaodaiG Jan 11, 2021

mpherman2 Jan 11, 2021

chaodaiG commented Jan 11, 2021

alvaroaleman Jan 11, 2021

mpherman2 Jan 12, 2021

alvaroaleman Jan 11, 2021 •

edited

Loading

chaodaiG Jan 11, 2021

petr-muller commented Jan 13, 2021

alvaroaleman Jan 13, 2021

mpherman2 Jan 13, 2021

alvaroaleman Jan 13, 2021

mpherman2 Jan 13, 2021

alvaroaleman Jan 13, 2021

mpherman2 Jan 13, 2021

alvaroaleman Jan 13, 2021

mpherman2 Jan 13, 2021

k8s-ci-robot commented Jan 13, 2021

chaodaiG commented Jan 13, 2021

stevekuznetsov commented Feb 10, 2021

stevekuznetsov Feb 10, 2021

mpherman2 Feb 10, 2021

stevekuznetsov Feb 10, 2021

mpherman2 Feb 11, 2021

Add check to make sure fork exists for autobump and if not create the fork #20428

Add check to make sure fork exists for autobump and if not create the fork #20428

Conversation

mpherman2 commented Jan 9, 2021

k8s-ci-robot commented Jan 9, 2021

mpherman2 commented Jan 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaodaiG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaodaiG commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petr-muller commented Jan 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 13, 2021

chaodaiG commented Jan 13, 2021

stevekuznetsov commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman Jan 11, 2021 •

edited

Loading