Release charts push #468

nicholasSUSE · 2024-08-09T20:50:18Z

Issue:

Solution:

After: #464

This command (release push charts <some_release_branch>),
will push the changes on the local branch,
and create a Pull Request against the given release branch.

Debug mode

Since it is a critical operation,
I implemented the debug mode which will guide the user on:

inspecting the commits that will be pushed.
inspecting the files that will be pushed.

Result example:

Without debug mode, this is an example PR created: rancher/charts#4333

The debug mode is too verbose to print here.

cmd/release/cmd/push.go

repository/repository.go

release/charts/charts.go

repository/repository.go

briandowns · 2024-08-24T02:31:59Z

cmd/release/cmd/push.go


+	"github.com/google/go-github/v39/github"
+	"github.com/spf13/cobra"
+


Remove this line. Imports should only be in 2 groups; stdlib and third party.

briandowns · 2024-08-24T02:33:15Z

cmd/release/cmd/push.go

+			releaseBranch string // given release branch
+
+			ctx context.Context // background context
+			t   string          // token


Can we rename this variable token?

briandowns · 2024-08-24T02:33:48Z

cmd/release/cmd/push.go

+		var (
+			releaseBranch string // given release branch
+
+			ctx context.Context // background context


This doesn't need to be declared here. Can be used below.

briandowns · 2024-08-24T02:34:12Z

cmd/release/cmd/push.go

+
+			ctx context.Context // background context
+			t   string          // token
+			ghc *github.Client  // github client


This also doesn't need a declaration. Can be used from the assignment below.

briandowns · 2024-08-24T02:34:51Z

exec/exec.go

+// UserInput will ask for user input with a given title
+func UserInput(title string) bool {
+	var input string
+	var err error


Let's remove this declaration.

briandowns · 2024-08-24T02:41:11Z

repository/repository.go

 	"os"
 	"strings"
 	"time"

 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/config"
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/plumbing/object"
+	"github.com/go-git/go-git/v5/plumbing/transport/http"
+


Remove empty lines and reformat.

briandowns · 2024-08-24T02:41:48Z

repository/repository.go

@@ -470,3 +476,169 @@ To find more information on specific steps, please see documentation [here](http
 - [ ] QA: Final validation of above PR and tracked through the linked ticket
 - [ ] PJM: Close the milestone in GitHub.
 `
+
+// GetUpstreamRemote will return the remote name for a given configured remote URL
+func GetUpstreamRemote(r *git.Repository, rURL string) (string, error) {


Let's remove "Get" from this function and call it "UpstramRemote". "Get" is a smell in Go.

briandowns · 2024-08-24T02:42:40Z

repository/repository.go

+}
+
+// getCommits returns the commits beginning at the given hash
+func getCommits(r *git.Repository, h plumbing.Hash) ([]*object.Commit, error) {


Same as previous "get" comment.

briandowns · 2024-08-24T02:43:17Z

repository/repository.go

+
+	limit := 100
+	count := 0
+	err = iter.ForEach(func(c *object.Commit) error {


This error should be checked for nil first and then check for a certain kind of error.

briandowns · 2024-08-24T02:43:43Z

repository/repository.go

+		err = nil // Reset error if it was forced by the limit
+	}
+
+	return commits, err


Let's return nil here for the error explicitly if there is not error.

briandowns · 2024-08-24T03:54:54Z

release/charts/chart_management.go

@@ -21,15 +21,20 @@ type asset struct {

 // ChartArgs will return the list of available charts in the current branch
 func ChartArgs(ctx context.Context, c *config.ChartsRelease) ([]string, error) {
-	assets := filepath.Join(c.Workspace, "assets")
+	var (
+		assets string


None of these appear to be needed to be pre declared.

briandowns · 2024-08-24T03:55:59Z

release/charts/charts.go

 )

+// body is the default PR body for the charts release PR
+const body string = `


Let's rename this chartsReleasePRBody

briandowns · 2024-08-24T03:57:27Z

release/charts/charts.go

+//   - b: release branch name.
+//   - t: token from github.
+//   - d: debug mode.
+func Push(ctx context.Context, c *config.ChartsRelease, u *config.User, ghc *github.Client, b, t string, d bool) (string, error) {


Please name the argument variables with names that indicate what they're used for and then you can remove the description for them above.

briandowns · 2024-08-24T03:57:52Z

release/charts/charts.go

+
+	// create a new PR
+	pr := &github.NewPullRequest{
+		Title:               github.String(fmt.Sprintf("[%s] batch release", b)),


Please concatenate the strings rather than using the sprintf function.

briandowns · 2024-08-24T03:59:29Z

release/charts/charts.go

@@ -93,3 +190,47 @@ func runChartsBuild(chartsRepoPath string, args ...string) ([]byte, error) {

 	return output, nil
 }
+
+// debugPullRequest will prompt the user to check the files and commits that will be pushed to the remote repository
+func debugPullRequest(r *git.Repository, remote, b string) error {


Please rename the b variable to be descriptive of what it is. In Go, using a single character symbol is fine when the context of the variable is easily derived and the variable is used close to its initialization.

briandowns · 2024-08-24T04:00:09Z

repository/repository.go

@@ -470,3 +476,169 @@ To find more information on specific steps, please see documentation [here](http
 - [ ] QA: Final validation of above PR and tracked through the linked ticket
 - [ ] PJM: Close the milestone in GitHub.
 `
+
+// GetUpstreamRemote will return the remote name for a given configured remote URL
+func GetUpstreamRemote(r *git.Repository, rURL string) (string, error) {


Please rename rURL to remoteURL.

briandowns · 2024-08-24T23:31:53Z

cmd/release/cmd/push.go

+
+		var (
+			releaseBranch string
+			found         bool


This variable doesn't need to be declared or even used below. The function call be be directly in the if statement.

briandowns · 2024-08-24T23:32:28Z

cmd/release/cmd/push.go

+			return err
+		}
+
+		fmt.Printf("Pull request created: %s\n", prURL)


Please concatenate this string rather than using the printf function.

briandowns · 2024-08-26T21:30:45Z

repository/repository.go

+// DiffLocalToRemote will get the commits from the local branch and from the target remote branch,
+// will return the commits that are present in the local branch but not in the remote branch.
+func DiffLocalToRemote(r *git.Repository, remote, releaseBranch string) error {
+	var (


It doesn't appear as though any of these variables need to be defined here.

briandowns · 2024-08-26T21:31:22Z

repository/repository.go

+
+// findUniqueCommits returns the commits that are in the first reference but not in the second.
+func findUniqueCommits(r *git.Repository, localRef, remoteRef *plumbing.Reference) ([]*object.Commit, error) {
+	var (


These variables don't need to be defined.

nicholasSUSE marked this pull request as ready for review August 9, 2024 20:54

nicholasSUSE force-pushed the release-charts-push branch 4 times, most recently from 08ba503 to bafa827 Compare August 10, 2024 15:28

nicholasSUSE requested a review from briandowns August 12, 2024 16:47

nicholasSUSE added 6 commits August 12, 2024 14:13

adding release push charts command

4c808bc

updating release/charts with push execution and pr creation

ba88a18

adding repository specific functions for handling go-git and ghClient

c55da34

updating arguments.go with new branch values and refactoring

04d0dee

adding user input function

dbdfdfe

fixes

f7eb14d

nicholasSUSE force-pushed the release-charts-push branch from bafa827 to f7eb14d Compare August 12, 2024 17:13

nicholasSUSE requested a review from tashima42 August 12, 2024 19:00

nicholasSUSE force-pushed the release-charts-push branch from d8e0d9a to 1c5c6fb Compare August 12, 2024 20:11

nicholasSUSE added 2 commits August 12, 2024 17:14

updating docs

d25f358

updating arg of list.go to match release push and release update charts

113b2d1

nicholasSUSE force-pushed the release-charts-push branch from 1c5c6fb to 113b2d1 Compare August 12, 2024 20:14

tashima42 reviewed Aug 12, 2024

View reviewed changes

cmd/release/cmd/push.go Outdated Show resolved Hide resolved

repository/repository.go Outdated Show resolved Hide resolved

release/charts/charts.go Outdated Show resolved Hide resolved

tashima42 reviewed Aug 12, 2024

View reviewed changes

repository/repository.go Outdated Show resolved Hide resolved

nicholasSUSE added 3 commits August 12, 2024 18:45

using root debug flag

f146a0e

updating comments for push() func

a5c65af

limiting amount of commits to inspect

a3658ad

nicholasSUSE force-pushed the release-charts-push branch from e4fa45f to a3658ad Compare August 12, 2024 22:00

nicholasSUSE requested a review from tashima42 August 12, 2024 22:01

small fix at default PR description

4639fd1

tashima42 approved these changes Aug 23, 2024

View reviewed changes

briandowns requested changes Aug 24, 2024

View reviewed changes

briandowns reviewed Aug 24, 2024

View reviewed changes

nicholasSUSE force-pushed the release-charts-push branch from a3da91e to 16b2c2c Compare August 26, 2024 21:26

nicholasSUSE requested a review from briandowns August 26, 2024 21:27

briandowns requested changes Aug 26, 2024

View reviewed changes

fixes after brian review

3668c03

nicholasSUSE force-pushed the release-charts-push branch from 16b2c2c to 3668c03 Compare August 26, 2024 21:38

briandowns approved these changes Aug 26, 2024

View reviewed changes

nicholasSUSE merged commit 6ccddb4 into rancher:master Aug 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release charts push #468

Release charts push #468

nicholasSUSE commented Aug 9, 2024 •

edited

Loading

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 24, 2024

briandowns Aug 26, 2024

briandowns Aug 26, 2024


		"github.com/google/go-github/v39/github"
		"github.com/spf13/cobra"

Release charts push #468

Release charts push #468

Conversation

nicholasSUSE commented Aug 9, 2024 • edited Loading

Issue:

Solution:

Debug mode

Result example:

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

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

nicholasSUSE commented Aug 9, 2024 •

edited

Loading