Make lifecycle assets #132

nicholasSUSE · 2024-05-21T23:57:37Z

Context

After discussing we all agreed that we would maintain a newer lifecycle for assets version on charts as stated in the documentation:
New Assets Lifecycle

The new lifecycle will only be applied to prod-v2.9+

Problem

It is a huge number of versions and assets, the work would be manual and prone to human errors.

Solution

Develop a script on charts-build-scripts to receive the branch version, calculate the rules and automatically remove all assets versions that do not belong to the lifecycle.

…ing to the lifecycle and unit-tests

It will load the filesystem, index.yaml, check necessary paths, parse information from index.yaml file and check git tree. Creating also parser.go which role will be parsing and converting information to a standard that is easier to work and debug. Creating also git.go with a check for the git tree status

…aml, check assets folder versions And later will execute the make remove for each asset version and commit the changes. The checking and populating the map of the assets folder versions are updated on parser.go.

Updating git.go with gitAddAndCommit

lucasmlp · 2024-05-22T17:40:05Z

main.go

 	// DefaultCacheEnvironmentVariable is the default environment variable that indicates that a cache should be used on pulls to remotes
 	DefaultCacheEnvironmentVariable = "USE_CACHE"
+	// DefaultDebugEnvironeventVariable is the default environment variable that indicates that debug mode should be enabled
+	DefaultDebugEnvironeventVariable = "DEBUG"


DefaultDebugEnvironmentVariable

lucasmlp · 2024-05-22T17:40:14Z

main.go

+		Name:        "debugFlag",
+		Usage:       "Enable debug mode",
+		Destination: &DebugMode,
+		EnvVar:      DefaultDebugEnvironeventVariable,


DefaultDebugEnvironmentVariable

lucasmlp · 2024-05-22T20:18:30Z

pkg/lifecycle/lifecycle.go

+	// Check if the assets folder and Helm index file exists in the repository
+	exists, err := filesystem.PathExists(dep.rootFs, path.RepositoryAssetsDir)
+	if err != nil || !exists {
+		return nil, fmt.Errorf("encountered error while checking if assets folder already exists in repository: %s", err)


If your condition is an OR, two cases might trigger the conditional.
err might not be null and the folder might not exist. There is no message for that.

lucasmlp · 2024-05-22T20:19:56Z

pkg/lifecycle/lifecycle.go

+		return nil, fmt.Errorf("encountered error while checking if assets folder already exists in repository: %s", err)
+	}
+	exists, err = filesystem.PathExists(dep.rootFs, path.RepositoryHelmIndexFile)
+	if err != nil || !exists {


If your condition is an OR, two cases might trigger the conditional.
err might not be null and the file might not exist. There is no message for that.

lucasmlp · 2024-05-22T20:22:28Z

pkg/lifecycle/lifecycle.go

+	}
+
+	// Git tree must be clean before proceeding with removing charts
+	clean, err := dep.checkIfGitIsCleanWrapper(debug)


Move this as the first step of the function. There is no need to run all that code if git is not clean at the beginning. That happens a lot with me while using the build-scripts

lucasmlp · 2024-05-23T14:28:07Z

pkg/lifecycle/lifecycle.go

+
+// removeVersionsAssets will iterate through assetsVersionsMap and remove the versions that are not in the lifecycle commiting the changes
+func (ld *Dependencies) removeVersionsAssets(debug bool) (map[string][]Asset, error) {
+	logrus.Info("Executing make remove")


IMO you should always check if git is clean first if your committing something later.

We have already checked it before on InitDependencies
After that, there is nothing changing the local file system.

lucasmlp · 2024-05-23T16:58:36Z

pkg/lifecycle/lifecycle_test.go

+		_, err := dep.removeVersionsAssets(false)
+
+		// Assert
+		if err == nil {


The error message is not coherent with the conditional.
Conditional says err equals nil

lucasmlp · 2024-05-23T16:58:43Z

pkg/lifecycle/lifecycle_test.go

+
+		// Assert
+		if err == nil {
+			t.Errorf("removeVersionsAssets returned an error: %v", err)


The error message is not coherent with the conditional.
Conditional says err equals nil

lucasmlp · 2024-05-23T16:58:50Z

pkg/lifecycle/lifecycle_test.go

+
+		// Assert
+		if err == nil {
+			t.Errorf("removeVersionsAssets returned an error: %v", err)


The error message is not coherent with the conditional.
Conditional says err equals nil

lucasmlp · 2024-05-23T17:12:29Z

pkg/lifecycle/lifecycle_test.go

A general comment about test files. IMO you should check the objects being returned with assert in the success cases and the error messages in the failure cases.

What you are suggesting is to use: "github.com/stretchr/testify/assert"

I am using the built-in "testing" package of go without 3rd party software.
The "testing" package does not have an assert method.

I prefer to use "github.com/stretchr/testify/assert" package when testing database and API calls that need to assert
big/complex objects and specific errors.

In this case, I did not want to use it because the objects are simple and most of the errors don't matter as long as they are errors failing on that specific condition.

I am trying not to over-engineer this.

nicholasSUSE added 2 commits May 21, 2024 20:28

creating lifecycle-assets cli option, args and flags

4cdf054

creating versions rules that will be used to remove assets not belong…

1831546

…ing to the lifecycle and unit-tests

nicholasSUSE force-pushed the make-lifecycle-assets branch from e0c9c9b to df8812b Compare May 22, 2024 00:03

nicholasSUSE requested a review from a team May 22, 2024 00:10

nicholasSUSE added 6 commits May 22, 2024 12:14

adding unit-tests for parser.go, mocking files and folder structure

c0da1c4

executing go mod tidy for new packages previously added

0170597

adding remove versions loop with checks and the make remove executor.

b5848a3

Updating git.go with gitAddAndCommit

unit-tests for lifecycle.go

901f69a

nicholasSUSE force-pushed the make-lifecycle-assets branch from 60bddef to afd9d47 Compare May 22, 2024 15:15

nicholasSUSE changed the title ~~Make lifecycle assets {WIP}~~ Make lifecycle assets May 22, 2024

nicholasSUSE marked this pull request as ready for review May 22, 2024 15:32

nicholasSUSE force-pushed the make-lifecycle-assets branch from afd9d47 to e337f5d Compare May 22, 2024 20:21

improve comments and log for versions.rules.go

bac6d4e

nicholasSUSE force-pushed the make-lifecycle-assets branch from e337f5d to bac6d4e Compare May 22, 2024 20:46

lucasmlp suggested changes May 23, 2024

View reviewed changes

first review corrections

125a7f0

lucasmlp approved these changes May 23, 2024

View reviewed changes

nicholasSUSE merged commit f46bd8c into rancher:master May 23, 2024

nicholasSUSE mentioned this pull request May 23, 2024

[prod-v2.9] adding new charts-build-script option for cleaning old assets rancher/charts#3959

Merged

Make lifecycle assets #132

Make lifecycle assets #132

Uh oh!

Conversation

nicholasSUSE commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Problem

Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nicholasSUSE commented May 21, 2024 •

edited

Loading