Feature Implementation of Automatic forward port of asset versions, i.e: make auto-forward-port #138

nicholasSUSE · 2024-06-27T01:18:19Z

Problem

Forward porting each asset version individually is an effortful manual work and prone to human error.

For each asset version, this was the manual process:

Check if the asset version is not present at the production branch and present at development branch.
Open a new local branch
Clean release.yaml
Add and commit.
execute make forward-port
Add and commit.
Repeat it for every asset version and it's dependencies
Push to a forked repository.
Open a Pull Request to the target production branch.
Review and approve PR.
Monitor git-mirror.

Solution

Implement automation to reduce the amount of manual work and human intervention.
This solution automates the above mentioned steps: (1 -> 8)

Results

These are the automated pushes generated by this automation:

rancher/charts#4137
rancher/charts#4138
rancher/charts#4139
rancher/charts#4140
rancher/charts#4141
rancher/charts#4142
rancher/charts#4143
rancher/charts#4144
rancher/charts#4145
rancher/charts#4146
rancher/charts#4147
rancher/charts#4148
rancher/charts#4149
rancher/charts#4150
rancher/charts#4151
rancher/charts#4152
rancher/charts#4153
rancher/charts#4154
rancher/charts#4155
rancher/charts#4156
rancher/charts#4157
rancher/charts#4158
rancher/charts#4159
rancher/charts#4160

Returning *Status to be used by another package.

Improving timestamp for logs creation.

Updating references to version.rules.go at: - status.go - logs.go - versions.rules_test.go

Removing old git.go inside pkg/lifecycle

… and releasing charts. Created data structures and method for initializing dependencies.

receiving the status object from CheckLifecycleStatusAndSave(), parse the assetsToBeForwardPorted map, into a slice of bash script commands. Sort it alphabetically and by version.

…harts into PRs to be pushed later. Rules: - rancher-istio = 1 chart - rancher-eks-operator + rancher-eks-operator-crd = 1 chart - fleet + fleet-crd + fleet-agent = 1 chart 1 chart = 1 branch = 1 PR checkChartCommonPrefixes is a more complex set of rules, but basically it helps enforce the ones mentioned above, however removing "rancher-" prefix which is very common among many charts, but it does not mean that they are the same.

These are the core rules of this feature.

Steps of the manual process implemented by this automation: 1. Create a new branch from production branch 2. Clean release.yaml file 3. Add and commit 4. execute bash script: "make forward-port CHART=<chart> VERSION=<version> BRANCH=<dev branch> UPSTREAM=<remote_name> 5. Add and commit 6. Repeat it to all assets versions that belong to the same chart. 7. Push to forked repository This code is not yet implementing the push to a forked repository, it will be implemented in the next commit.

…for it

recena · 2024-06-27T21:10:31Z

@nicholasSUSE You can use the DRAFT feature instead of [wip] prefix.

briandowns · 2024-07-02T21:36:36Z

pkg/lifecycle/versions.rules.go

@@ -146,5 +146,5 @@ func (vr *VersionRules) CheckChartVersionToRelease(chartVersion string) (bool, e
 		logrus.Errorf("failed to check version to release for chartVersion:%s with error:%v", chartVersion, err)
 		return false, err
 	}
-	return chartVersionInt == (vr.maxVersion - 1), nil
+	return chartVersionInt == (vr.MaxVersion - 1), nil


The parenthesis aren't necessary.

I agree it is not necessary.

I intentionally wrote this way for easier reading.
Can we keep this one?

briandowns · 2024-07-02T21:37:23Z

pkg/auto/auto.go

+
+// createForwardPortCommands will create the forward port script commands for each asset and version,
+// and return a sorted slice of commands
+func (fp *ForwardPort) createForwardPortCommands(chart string) ([]Command, error) {


Prefer using a single character for method receivers.

Fixing also for forward.port.go

briandowns · 2024-07-02T21:39:26Z

pkg/auto/auto.go

+
+	upstreamRemote, ok := fp.git.Remotes["https://github.com/rancher/charts"]
+	if !ok {
+		logrus.Error("upstream remote not found; you need to have the upstream remote configured in your git repository (https://github.com/rancher/charts)")


This should be made into a value that can be printed here and returned below rather than being repeated.

var errUpstreamNotFound = errors.New("upstream remote not found; you need to have the upstream remote configured in your git repository (https://github.com/rancher/charts)")

briandowns · 2024-07-02T21:40:13Z

pkg/auto/auto.go

+func checkIfChartChanged(lastChart, currentChart string) bool {
+	// Check if the current chart is different from the last chart
+	sameCharts := (lastChart == currentChart)
+	if sameCharts {


This can be simplified: Remove line 113 and add the conditional check to line 114.

briandowns · 2024-07-02T21:40:40Z

pkg/auto/auto.go

+	}
+
+	// Check for special edge cases
+	edgeCase := checkEdgeCasesIfChartChanged(lastChart, currentChart)


I think this can also be simplified as:

return !checkEdgeCasesIfChartChanged(lastChart, currentChart)

briandowns · 2024-07-02T21:41:20Z

pkg/auto/auto.go

+		return true
+	}
+	if equalCounter == 1 && strings.HasPrefix("rancher-", lastParts[0]) &&
+		minLength < 3 {


Move tyhis up a line. Go doesn't care at all about line length, per Rob Pike.

briandowns · 2024-07-02T21:41:43Z

pkg/auto/auto.go

+		return err
+	}
+	// create new branch and checkout
+	err = fp.git.CreateAndCheckoutBranch(branch)


Suggested change

err = fp.git.CreateAndCheckoutBranch(branch)

return fp.git.CreateAndCheckoutBranch(branch)

briandowns · 2024-07-02T21:44:05Z

pkg/auto/auto.go

+	}
+
+	// File exists, open it with O_TRUNC to erase its content
+	file, err := os.OpenFile("release.yaml", os.O_WRONLY|os.O_TRUNC, 0644)


Suggested change

file, err := os.OpenFile("release.yaml", os.O_WRONLY|os.O_TRUNC, 0644)

if err := os.Truncate("release.yaml, 0); err != nil {

return err

}

The defer can be removed.

Wooow, did not know that.
Fixing.

pkg/auto/auto.go

briandowns · 2024-07-02T21:46:28Z

pkg/auto/forward.port.go

@@ -0,0 +1,161 @@
+package auto


This file isn't named according to Go standards. Replace the first . with an _. And do this for any other file names similarly.

briandowns · 2024-07-02T21:52:17Z

main.go

@@ -43,6 +44,8 @@ const (
 	DefaultDebugEnvironmentVariable = "DEBUG"
 	// DefaultBranchVersionEnvironmentVariable is the default environment variable that indicates the branch version to compare against
 	DefaultBranchVersionEnvironmentVariable = "BRANCH_VERSION"
+	// DefaultForkEnvironmentVariable is the default environment variable that indicates the fork URL


None of these need to be exported since they can't be imported from main. Same for the new variable below.

briandowns · 2024-07-02T21:53:51Z

main.go

+func autoForwardPort(c *cli.Context) {
+
+	if ForkURL == "" {
+		logrus.Fatalf("FORK environment variable must be set to run auto-forward-port")


Suggested change

logrus.Fatalf("FORK environment variable must be set to run auto-forward-port")

logrus.Fatal("FORK environment variable must be set to run auto-forward-port")

main.go

briandowns · 2024-07-03T20:39:17Z

main.go

@@ -71,6 +74,8 @@ var (
 	CacheMode = false


In another pr this should be updated to match the ones above using the 0 value declaration rather than this declaration to false. The 0 value for bools is false but moreover like code should be consistent. Which means the new variable below should be defined as type string since the 0 value for string is "".

Will also need to be unexported.

I created this issue so we don't lose track of this technical debt.
See if you agree please.

rancher/ecm-distro-tools#438

briandowns · 2024-07-03T20:42:10Z

pkg/auto/auto.go

+func prepareReleaseYaml() error {
+	// Check if the file exists
+	_, err := os.Stat("release.yaml")
+	if os.IsNotExist(err) {


The nil check on err should always be done first and if it's not nil, then inside that scope further error checking/handling should occur.

briandowns · 2024-07-03T20:47:07Z

pkg/git/utils.go

+		return false // Remote URL is empty
+	}
+	urlPart1, urlPart2 := extractCommonParts(upstreamURL, remoteURL)
+	return (urlPart1 == "https://github.com" && urlPart2 == repo)


Suggested change

return (urlPart1 == "https://github.com" && urlPart2 == repo)

return urlPart1 == "https://github.com" && urlPart2 == repo

nicholasSUSE added 16 commits June 26, 2024 21:15

adding *.out to .gitignore

d5812fe

adding make auto-forward-port feature to CLI commands

63c4582

improving error handling and logs for main CLI commands recently created

ca5b998

Making AssetsToBeForwardPorted public to be used by another package.

3c6c6e5

Returning *Status to be used by another package.

Preparing logs from lifecycle package to be used by other packages.

26ea790

Improving timestamp for logs creation.

Preparing version.rules.go to be used publicly by other packages

2deb7b2

Updating references to version.rules.go at: - status.go - logs.go - versions.rules_test.go

making Asset struct field Version accessible by other packages

e9dc444

Creating git package to be used by more than one package

35dd693

Removing old git.go inside pkg/lifecycle

updating references to new git package

5e30c3d

updating logs.go and status.go to use public methods

78a6eb0

Adding auto package that will execute automations for forward-porting…

c437df3

… and releasing charts. Created data structures and method for initializing dependencies.

creating initial methods to execute auto forward ports.

af1489b

receiving the status object from CheckLifecycleStatusAndSave(), parse the assetsToBeForwardPorted map, into a slice of bash script commands. Sort it alphabetically and by version.

adding unit-tests for creating Forward Port commands

5750bd6

adding unit-tests for organizing Pull Requests.

f0266ed

These are the core rules of this feature.

nicholasSUSE requested a review from a team as a code owner June 27, 2024 01:18

changing how to handle chart special cases, improving the unit-tests …

cc4abdb

…for it

adding CLI argument option for forked remote urls, adding unit-tests

f2936e3

briandowns reviewed Jul 2, 2024

View reviewed changes

pkg/auto/auto.go Show resolved Hide resolved

briandowns reviewed Jul 2, 2024

View reviewed changes

main.go Show resolved Hide resolved

1st batch of review fixes

f519918

nicholasSUSE mentioned this pull request Jul 3, 2024

[prod-v2.9] Auto forward port rancher vsphere cpi 2.9 rancher/charts#4175

Merged

nicholasSUSE added 2 commits July 3, 2024 16:18

2nd batch of review fixes; also improving error logging

cddff07

3rd batch of review fixes

4264d70

nicholasSUSE requested a review from briandowns July 3, 2024 20:22

briandowns reviewed Jul 3, 2024

View reviewed changes

nicholasSUSE requested a review from briandowns July 3, 2024 23:54

briandowns approved these changes Jul 5, 2024

View reviewed changes

nicholasSUSE merged commit fc0e5dd into rancher:master Jul 5, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Implementation of Automatic forward port of asset versions, i.e: make auto-forward-port #138

Feature Implementation of Automatic forward port of asset versions, i.e: make auto-forward-port #138

nicholasSUSE commented Jun 27, 2024 •

edited

Loading

recena commented Jun 27, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024 •

edited

Loading

nicholasSUSE Jul 3, 2024

briandowns Jul 2, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 3, 2024

briandowns Jul 3, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 3, 2024

nicholasSUSE Jul 3, 2024

briandowns Jul 3, 2024

nicholasSUSE Jul 3, 2024

	err = fp.git.CreateAndCheckoutBranch(branch)
	return fp.git.CreateAndCheckoutBranch(branch)

	logrus.Fatalf("FORK environment variable must be set to run auto-forward-port")
	logrus.Fatal("FORK environment variable must be set to run auto-forward-port")

	return (urlPart1 == "https://github.com" && urlPart2 == repo)
	return urlPart1 == "https://github.com" && urlPart2 == repo

Feature Implementation of Automatic forward port of asset versions, i.e: make auto-forward-port #138

Feature Implementation of Automatic forward port of asset versions, i.e: make auto-forward-port #138

Conversation

nicholasSUSE commented Jun 27, 2024 • edited Loading

Problem

Solution

Results

recena commented Jun 27, 2024

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

briandowns Jul 2, 2024 • edited Loading

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 Jun 27, 2024 •

edited

Loading

briandowns Jul 2, 2024 •

edited

Loading