fix: Function "runPromote" is overly complex #1343

varshith257 · 2024-06-13T10:03:38Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The current runPromote function is too complex. Instead organised code into helper functions of runPromote function

Which issue(s) this PR fixes:

Fixes Function "runPromote" is overly complex #1177

Special notes for your reviewer:

Does this PR introduce a user-facing change?

No

linux-foundation-easycla · 2024-06-13T10:03:43Z

✅login: varshith257 / (65c091c)

The committers listed above are authorized under a signed CLA.

k8s-ci-robot · 2024-06-13T10:03:47Z

Welcome @varshith257!

It looks like this is your first PR to kubernetes-sigs/promo-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/promo-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-06-13T10:03:48Z

Hi @varshith257. 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 /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-sigs/prow repository.

varshith257 · 2024-06-13T10:30:45Z

cc/@cpanato @xmudrii

cpanato

/ok-to-test

cpanato · 2024-06-13T12:55:27Z

/ok-to-test

varshith257 · 2024-06-14T08:24:45Z

@saschagrunert PTAL

varshith257 · 2024-06-28T12:26:51Z

@xmudrii @cpanato Any review on it?

cpanato · 2024-06-28T12:47:15Z

@varshith257 i would like to run that, but I need more time
but im general you split the function in smaller ones, correct?

varshith257 · 2024-06-28T12:52:26Z

@cpanato YES! As described in the issue to make it code reusability and reduce the cognitive load of it to understand, I have split it into smaller helper functions of runPromote

varshith257 · 2024-07-22T13:00:10Z

@cpanato Can you have a look into it?

cpanato · 2024-07-22T13:36:01Z

@cpanato Can you have a look into it?

i am on PTO will be back end of august only

xmudrii · 2024-07-22T15:37:51Z

I'll do the initial review in the upcoming days
/assign

varshith257 · 2024-07-22T16:27:28Z

Thanks @xmudrii !

A quick info,
Do the project of refactoring of promo-tools project to support sig will be getting into Term 3 LFX Mentorship?

It has been cancelled in Term 2. I am one of the applicant for this project in Term 2 and eagerly waiting to mentor under the Kubernetes organisation

xmudrii · 2024-07-22T16:30:33Z

@varshith257 As far as I know, we don't have plans for submitting a project for the LFX Term 3, but follow our #sig-release Slack channel for the latest updates

varshith257 · 2024-07-22T16:49:16Z

@xmudrii I am unable to join the Kubernetes slack. It's showing I don't have an email account for the workspace.

Can I get an invite Link for the Kubernetes slack to join?

Mail id: [email protected]

xmudrii · 2024-07-22T17:01:37Z

@varshith257 Please use slack.k8s.io to register on the Kubernetes Slack.

xmudrii

Thank your for this PR and the initiative to improve our code base! However, in order for this PR to get merged, the refactored code must match the original behavior prior to the refactor. I left some comments in places where this is not the case, once this is fixed, we can merge this PR. I also left some comments pointing what you can improve in this PR.

xmudrii · 2024-07-23T12:37:34Z

cmd/kpromo/cmd/pr/pr.go

+	return nil
+}
+
+func prepareRepository(branchname string, opts *promoteOptions) (*git.Repo, error) {


Two nits:

given this is about the fork, maybe we should call it prepareFork

I have mixed feelings about branchname, I'd prefer branchName, although I see that the variable in rumPromote is called branchname

xmudrii · 2024-07-23T12:43:08Z

cmd/kpromo/cmd/pr/pr.go

+	return nil
+}
+
+func prepareRepository(branchname string, opts *promoteOptions) (*git.Repo, error) {


Additionally, I'd make all these functions that take promoteOptions as an argument, take promoteOptions as a receiver, e.g. func (opts *promoteOptions) prepareRepository (branchname string)

xmudrii · 2024-07-23T12:44:40Z

cmd/kpromo/cmd/pr/pr.go

@@ -232,78 +267,82 @@ func runPromote(opts *promoteOptions) error {
 	)

 	// Read the current manifest to check later if new images come up
-	oldlist := make([]byte, 0)


Why did you change this?

xmudrii · 2024-07-23T12:51:15Z

cmd/kpromo/cmd/pr/pr.go

 		}
-	}


The original code is in the mustRun block only up to this point. I believe this function should do the same.

xmudrii · 2024-07-23T13:06:04Z

cmd/kpromo/cmd/pr/pr.go

+		// add the modified manifest to staging
+		logrus.Debugf("Adding %s to staging area", imagesListPath)
+		if err := repo.Add(imagesListPath); err != nil {
+			return false, fmt.Errorf("adding image manifest to staging area: %w", err)
 		}


For some reason, this doesn't really feel like part of this function to me. Additionally, if you would put it in the runPronmote function, you could avoid passing repo and instead pass the value of repo.Dir().

xmudrii · 2024-07-23T13:08:19Z

cmd/kpromo/cmd/pr/pr.go

+	}()
+
+	// Handle manifests
+	modified, err := handleManifests(ctx, repo, opts)


handleManifests is a bit broad name, what do you think about calling it growManifest instead?

xmudrii · 2024-07-23T13:10:56Z

cmd/kpromo/cmd/pr/pr.go

+	if mustRun(opts, fmt.Sprintf("Push changes to user's fork at %s/%s?", opts.userFork, k8sioRepo)) {
+		logrus.Infof("Pushing manifest changes to %s/%s", opts.userFork, k8sioRepo)
 		if err := repo.PushToRemote(github.UserForkName, branchname); err != nil {
-			return fmt.Errorf("pushing %s to %s/%s: %w", github.UserForkName, userForkOrg, userForkRepo, err)
+			return fmt.Errorf("pushing %s to %s/%s: %w", github.UserForkName, opts.userFork, k8sioRepo, err)


This looks wrong to me, why did you replace userForkOrg with opts.userFork and userForkRepo with k8sioRepo?

xmudrii · 2024-07-23T13:13:22Z

cmd/kpromo/cmd/pr/pr.go

+		logrus.Infof("Exiting without creating a PR since changes were not pushed to %s/%s", opts.userFork, k8sioRepo)
 		return nil


The tool is supposed to exit here, however, it would continue running after this function. You should do something similar as in handleManifests or use os.Exit.

xmudrii · 2024-07-23T13:13:56Z

cmd/kpromo/cmd/pr/pr.go

+	gh := github.New()
+	pr, err := gh.CreatePullRequest(
+		git.DefaultGithubOrg, k8sioRepo, k8sioDefaultBranch,
+		fmt.Sprintf("%s:%s", opts.userFork, branchname),


Why did you replace userForkOrg with opts.userFork?

xmudrii · 2024-07-23T13:15:49Z

cmd/kpromo/cmd/pr/pr.go

+	pr, err := gh.CreatePullRequest(
+		git.DefaultGithubOrg, k8sioRepo, k8sioDefaultBranch,
+		fmt.Sprintf("%s:%s", opts.userFork, branchname),
+		"Image promotion for "+opts.project+" "+strings.Join(opts.tags, " / "),


This doesn't match the original behavior, the commit message can be extended in some case, e.g. with the releng: prefix.

k8s-ci-robot · 2024-07-23T13:17:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: varshith257
Once this PR has been reviewed and has the lgtm label, please ask for approval from xmudrii. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2024-10-17T20:46:01Z

PR needs rebase.

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-sigs/prow repository.

refactor runPromote func

65c091c

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 13, 2024

k8s-ci-robot added area/artifacts Issues or PRs related to the hosting of release artifacts for subprojects cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 13, 2024

k8s-ci-robot requested a review from jimangel June 13, 2024 10:03

k8s-ci-robot added the area/release-eng Issues or PRs related to the Release Engineering subproject label Jun 13, 2024

k8s-ci-robot requested a review from saschagrunert June 13, 2024 10:03

k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Jun 13, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2024

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 13, 2024

varshith257 marked this pull request as draft June 13, 2024 10:08

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2024

varshith257 marked this pull request as ready for review June 13, 2024 10:08

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2024

k8s-ci-robot requested a review from jrsapi June 13, 2024 10:08

cpanato reviewed Jun 13, 2024

View reviewed changes

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 Jun 13, 2024

varshith257 requested a review from cpanato June 13, 2024 16:03

k8s-ci-robot assigned xmudrii Jul 22, 2024

xmudrii requested changes Jul 23, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Function "runPromote" is overly complex #1343

fix: Function "runPromote" is overly complex #1343

varshith257 commented Jun 13, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jun 13, 2024 •

edited

Loading

k8s-ci-robot commented Jun 13, 2024

k8s-ci-robot commented Jun 13, 2024

varshith257 commented Jun 13, 2024

cpanato left a comment •

edited

Loading

cpanato commented Jun 13, 2024

varshith257 commented Jun 14, 2024

varshith257 commented Jun 28, 2024

cpanato commented Jun 28, 2024

varshith257 commented Jun 28, 2024

varshith257 commented Jul 22, 2024 •

edited

Loading

cpanato commented Jul 22, 2024

xmudrii commented Jul 22, 2024

varshith257 commented Jul 22, 2024 •

edited

Loading

xmudrii commented Jul 22, 2024

varshith257 commented Jul 22, 2024 •

edited

Loading

xmudrii commented Jul 22, 2024

xmudrii left a comment

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

xmudrii Jul 23, 2024

k8s-ci-robot commented Jul 23, 2024

k8s-ci-robot commented Oct 17, 2024

		logrus.Infof("Exiting without creating a PR since changes were not pushed to %s/%s", opts.userFork, k8sioRepo)
		return nil

fix: Function "runPromote" is overly complex #1343

Are you sure you want to change the base?

fix: Function "runPromote" is overly complex #1343

Conversation

varshith257 commented Jun 13, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

linux-foundation-easycla bot commented Jun 13, 2024 • edited Loading

k8s-ci-robot commented Jun 13, 2024

k8s-ci-robot commented Jun 13, 2024

varshith257 commented Jun 13, 2024

cpanato left a comment • edited Loading

Choose a reason for hiding this comment

cpanato commented Jun 13, 2024

varshith257 commented Jun 14, 2024

varshith257 commented Jun 28, 2024

cpanato commented Jun 28, 2024

varshith257 commented Jun 28, 2024

varshith257 commented Jul 22, 2024 • edited Loading

cpanato commented Jul 22, 2024

xmudrii commented Jul 22, 2024

varshith257 commented Jul 22, 2024 • edited Loading

xmudrii commented Jul 22, 2024

varshith257 commented Jul 22, 2024 • edited Loading

xmudrii commented Jul 22, 2024

xmudrii left a 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

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 Jul 23, 2024

k8s-ci-robot commented Oct 17, 2024

varshith257 commented Jun 13, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jun 13, 2024 •

edited

Loading

cpanato left a comment •

edited

Loading

varshith257 commented Jul 22, 2024 •

edited

Loading

varshith257 commented Jul 22, 2024 •

edited

Loading

varshith257 commented Jul 22, 2024 •

edited

Loading