Feature implementation: release list charts #456

nicholasSUSE · 2024-07-26T21:15:05Z

Issue:

Solution:

After implementing: rancher/charts-build-scripts#144

This is the integration between:

charts-build-scripts command lifecycle-status
ecm-distro-tools command release list charts

Result example:

The logs printed by lifecycle-status are printed to the terminal and the saved log files path is also printed in the terminal:

.
.
.

time=2024-07-26T18:11:33-03:00 level=info msg=Assets to be RELEASED

time=2024-07-26T18:11:33-03:00 level=info msg=rancher-istio

__________________________________________________________________
time=2024-07-26T18:11:33-03:00 level=info msg=Assets to be FORWARD-PORTED

time=2024-07-26T18:11:33-03:00 level=info msg=rancher-istio
time=2024-07-26T18:11:33-03:00 level=info msg=saving app state to state.json file
time=2024-07-26T18:11:33-03:00 level=info msg=saved state file successfully

generated log files for inspection at: 
/home/nick/WORK/SOFTWARE/go/src/github.com/release-work/charts/logs/

…cripts binary

briandowns · 2024-07-26T21:22:26Z

cmd/release/cmd/list.go

+	Short: "List Charts assets versions state for release process",
+	RunE: func(cmd *cobra.Command, args []string) error {
+
+		var branch, chart string = "", ""


Let's take advantage of the "0 value" and update this to be

var branch string var chart string

We don't need to explicitly set the new string to ""

briandowns · 2024-07-26T21:23:15Z

cmd/release/cmd/list.go

+			return errors.New("verify your config file, chart configuration not implemented correctly, you must insert workspace path and your forked repo url")
+		}
+
+		err := charts.ListLifecycleStatus(context.Background(), config, branch, chart)


You can return here and remove the code underneath.

cmd/release/cmd/list.go

briandowns · 2024-07-26T21:24:13Z

release/charts/charts.go

+	"os/exec"
+	"strings"
+
+	ecmConfig "github.com/rancher/ecm-distro-tools/cmd/release/config"


Does this need the import alias?

@briandowns
To be honest, I copied it from:
release/k3s/k3s.go but I can remove it.
I just wanted to follow the pattern that was already there from the example I caught.

Let's avoid import aliases if the imported package name doesn't conflict with any other imported or existing symbol.

briandowns · 2024-07-26T21:24:21Z

release/charts/charts.go

+
+// ListLifecycleStatus prints the lifecycle status of the charts
+func ListLifecycleStatus(ctx context.Context, c *ecmConfig.ChartsRelease, branch, chart string) error {
+


Remove this new line.

release/charts/charts.go

briandowns · 2024-07-26T21:25:44Z

release/charts/charts.go

+	}
+
+	// change working dir to the charts repo
+	err = os.Chdir(chartsRepoPath)


Move the if check up a line and scope lock the err value. Please do this for all calls that return a single err value.

release/charts/charts.go

briandowns · 2024-07-26T21:29:38Z

release/charts/charts.go

+	}
+
+	err := executeChartsBuildScripts(c.Workspace, "lifecycle-status", branchArg, chartArg)
+	if err != nil {


Let's rename this to: runChartsBuild. The previous name leaks implementation detail that could change in the future. This new name still conveys the idea and allows us to change from scripts to programs and exec model.

Not sure if this isn't updating but it doesn't appear as though this has been completed.

briandowns · 2024-07-26T21:58:00Z

release/charts/charts.go

+		return err
+	}
+
+	fmt.Printf("generated log files for inspection at: \n%s\n", c.Workspace+"/logs/")


Let's remove this in favor of printing output from the calling code of this function. Library code shouldn't be putting anything to a log or STDOUT unless in debug.

@briandowns I removed the print from here to the parent calling function, please check if this is right now.

nicholasSUSE · 2024-07-29T16:26:17Z

@briandowns all fixes implemented.

briandowns · 2024-07-31T18:47:56Z

cmd/release/cmd/list.go

+	Use:   "charts [branch] [charts](optional)",
+	Short: "List Charts assets versions state for release process",
+	RunE: func(cmd *cobra.Command, args []string) error {
+


Small thing but remove this empty line.

briandowns · 2024-07-31T18:49:35Z

release/charts/charts.go

+	return response, nil
+}
+
+func runChartsBuildScripts(chartsRepoPath string, args ...string) ([]byte, error) {


I thought I had left a comment about changing the name but I guess not. Please change this to be runChartsBuild

briandowns · 2024-08-02T20:43:49Z

release/charts/charts.go

+	// save current working dir
+	ecmWorkDir, err := os.Getwd()
+	if err != nil {
+		return []byte{}, err


I think we can/should return nil here rather than having an unnecessary allocation.

adding release list charts sub command

c440437

nicholasSUSE requested review from brooksn, briandowns and tashima42 July 26, 2024 21:15

nicholasSUSE changed the title ~~Release list charts~~ Feature implementation: release list charts Jul 26, 2024

nicholasSUSE force-pushed the release-list-charts branch from dd96c3e to 34b503c Compare July 26, 2024 21:23

adding charts release package with direct execution to charts-build-s…

2ef133d

…cripts binary

nicholasSUSE force-pushed the release-list-charts branch from 34b503c to 2ef133d Compare July 26, 2024 21:25

briandowns requested changes Jul 26, 2024

View reviewed changes

nicholasSUSE force-pushed the release-list-charts branch 2 times, most recently from d0537ea to 937541b Compare July 26, 2024 21:46

briandowns requested changes Jul 26, 2024

View reviewed changes

fixes

cd5720a

nicholasSUSE force-pushed the release-list-charts branch from 937541b to cd5720a Compare July 29, 2024 16:25

nicholasSUSE requested a review from briandowns July 29, 2024 16:26

fixes 2

f94a604

briandowns requested changes Jul 31, 2024

View reviewed changes

rafaelbreno approved these changes Aug 2, 2024

View reviewed changes

briandowns requested changes Aug 2, 2024

View reviewed changes

fixes 3

5abebe1

nicholasSUSE force-pushed the release-list-charts branch from b192b2d to 5abebe1 Compare August 2, 2024 20:46

nicholasSUSE requested a review from briandowns August 2, 2024 20:46

briandowns approved these changes Aug 2, 2024

View reviewed changes

nicholasSUSE merged commit 0e0019a into rancher:master Aug 2, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature implementation: release list charts #456

Feature implementation: release list charts #456

nicholasSUSE commented Jul 26, 2024 •

edited

Loading

briandowns Jul 26, 2024

briandowns Jul 26, 2024

briandowns Jul 26, 2024

nicholasSUSE Jul 26, 2024 •

edited

Loading

briandowns Jul 26, 2024

briandowns Jul 26, 2024

briandowns Jul 26, 2024

briandowns Jul 26, 2024

briandowns Jul 30, 2024

briandowns Jul 26, 2024

nicholasSUSE Jul 30, 2024

nicholasSUSE commented Jul 29, 2024

briandowns Jul 31, 2024

briandowns Jul 31, 2024

briandowns Aug 2, 2024


		// ListLifecycleStatus prints the lifecycle status of the charts
		func ListLifecycleStatus(ctx context.Context, c *ecmConfig.ChartsRelease, branch, chart string) error {

Feature implementation: release list charts #456

Feature implementation: release list charts #456

Conversation

nicholasSUSE commented Jul 26, 2024 • edited Loading

Issue:

Solution:

Result example:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasSUSE Jul 26, 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

nicholasSUSE commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasSUSE commented Jul 26, 2024 •

edited

Loading

nicholasSUSE Jul 26, 2024 •

edited

Loading