Merge 1604c48526 into 42f78ba60e

pkg/action/rollback.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"time"
+	"unicode/utf8"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -31,6 +32,8 @@ import (
 	"helm.sh/helm/v4/pkg/storage/driver"
 )
 
+const MaxDescriptionLength = 256
+
 // Rollback is the action for rolling back to a given release.
 //
 // It provides the implementation of 'helm rollback'.
@@ -59,6 +62,8 @@ type Rollback struct {
 	ServerSideApply string
 	CleanupOnFail   bool
 	MaxHistory      int // MaxHistory limits the maximum number of revisions saved per release
+	// Description is the description of this rollback operation
+	Description string
 }
 
 // NewRollback creates a new Rollback object with the given configuration.
@@ -72,6 +77,10 @@ func NewRollback(cfg *Configuration) *Rollback {
 
 // Run executes 'helm rollback' against the given release.
 func (r *Rollback) Run(name string) error {
+	if descLen := utf8.RuneCountInString(r.Description); descLen > MaxDescriptionLength {
+		return fmt.Errorf("description must be %d characters or less, got %d", MaxDescriptionLength, descLen)
+	}
+
 	if err := r.cfg.KubeClient.IsReachable(); err != nil {
 		return err
 	}
@@ -169,6 +178,12 @@ func (r *Rollback) prepareRollback(name string) (*release.Release, *release.Rele
 		return nil, nil, false, err
 	}
 
+	// Determine the description for this rollback
+	description := r.Description
+	if description == "" {
+		description = fmt.Sprintf("Rollback to %d", previousVersion)
+	}
+
 	// Store a new release object with previous release's configuration
 	targetRelease := &release.Release{
 		Name:      name,
@@ -182,7 +197,7 @@ func (r *Rollback) prepareRollback(name string) (*release.Release, *release.Rele
 			Notes:         previousRelease.Info.Notes,
 			// Because we lose the reference to previous version elsewhere, we set the
 			// message here, and only override it later if we experience failure.
-			Description: fmt.Sprintf("Rollback to %d", previousVersion),
+			Description: description,
 		},
 		Version:     currentRelease.Version + 1,
 		Labels:      previousRelease.Labels,

pkg/action/rollback_test.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"io"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -27,14 +28,26 @@ import (
 
 	"helm.sh/helm/v4/pkg/kube"
 	kubefake "helm.sh/helm/v4/pkg/kube/fake"
+	"helm.sh/helm/v4/pkg/release/common"
 )
 
+func rollbackAction(t *testing.T) *Rollback {
+	t.Helper()
+	config := actionConfigFixture(t)
+	rollAction := NewRollback(config)
+	return rollAction
+}
+
 func TestNewRollback(t *testing.T) {
+	is := assert.New(t)
 	config := actionConfigFixture(t)
-	client := NewRollback(config)
 
-	assert.NotNil(t, client)
-	assert.Equal(t, config, client.cfg)
+	rollback := NewRollback(config)
+
+	is.NotNil(rollback)
+	is.Equal(config, rollback.cfg)
+	is.Equal(DryRunNone, rollback.DryRunStrategy)
+	is.Empty(rollback.Description)
 }
 
 func TestRollbackRun_UnreachableKubeClient(t *testing.T) {
@@ -83,3 +96,219 @@ func TestRollback_WaitOptionsPassedDownstream(t *testing.T) {
 	// Verify that WaitOptions were passed to GetWaiter
 	is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be passed to GetWaiter")
 }
+
+func TestRollback_WithDescription(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	rollAction := rollbackAction(t)
+
+	// Create two releases - version 1 (superseded) and version 2 (deployed)
+	rel1 := releaseStub()
+	rel1.Name = "test-release"
+	rel1.Version = 1
+	rel1.Info.Status = common.StatusSuperseded
+	rel1.ApplyMethod = "csa" // client-side apply
+	req.NoError(rollAction.cfg.Releases.Create(rel1))
+
+	rel2 := releaseStub()
+	rel2.Name = "test-release"
+	rel2.Version = 2
+	rel2.Info.Status = common.StatusDeployed
+	rel2.ApplyMethod = "csa" // client-side apply
+	req.NoError(rollAction.cfg.Releases.Create(rel2))
+
+	// Set custom description
+	customDescription := "Rollback due to critical bug in version 2"
+	rollAction.Description = customDescription
+	rollAction.Version = 1
+	rollAction.ServerSideApply = "false" // Disable server-side apply for testing
+
+	err := rollAction.Run("test-release")
+	req.NoError(err)
+
+	// Get the new release (version 3)
+	newReleasei, err := rollAction.cfg.Releases.Get("test-release", 3)
+	req.NoError(err)
+	newRelease, err := releaserToV1Release(newReleasei)
+	req.NoError(err)
+
+	// Verify the custom description was set
+	is.Equal(customDescription, newRelease.Info.Description)
+}
+
+func TestRollback_DefaultDescription(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	rollAction := rollbackAction(t)
+
+	// Create two releases - version 1 (superseded) and version 2 (deployed)
+	rel1 := releaseStub()
+	rel1.Name = "test-release-default"
+	rel1.Version = 1
+	rel1.Info.Status = common.StatusSuperseded
+	rel1.ApplyMethod = "csa" // client-side apply
+	req.NoError(rollAction.cfg.Releases.Create(rel1))
+
+	rel2 := releaseStub()
+	rel2.Name = "test-release-default"
+	rel2.Version = 2
+	rel2.Info.Status = common.StatusDeployed
+	rel2.ApplyMethod = "csa" // client-side apply
+	req.NoError(rollAction.cfg.Releases.Create(rel2))
+
+	// Don't set a description, rely on default
+	rollAction.Version = 1
+	rollAction.ServerSideApply = "false" // Disable server-side apply for testing
+
+	err := rollAction.Run("test-release-default")
+	req.NoError(err)
+
+	// Get the new release (version 3)
+	newReleasei, err := rollAction.cfg.Releases.Get("test-release-default", 3)
+	req.NoError(err)
+	newRelease, err := releaserToV1Release(newReleasei)
+	req.NoError(err)
+
+	// Verify the default description was set
+	is.Equal("Rollback to 1", newRelease.Info.Description)
+}
+
+func TestRollback_EmptyDescription(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	rollAction := rollbackAction(t)
+
+	// Create two releases - version 1 (superseded) and version 2 (deployed)
+	rel1 := releaseStub()
+	rel1.Name = "test-release-empty"
+	rel1.Version = 1
+	rel1.Info.Status = common.StatusSuperseded
+	rel1.ApplyMethod = "csa" // client-side apply
+	req.NoError(rollAction.cfg.Releases.Create(rel1))
+
+	rel2 := releaseStub()
+	rel2.Name = "test-release-empty"
+	rel2.Version = 2
+	rel2.Info.Status = common.StatusDeployed
+	rel2.ApplyMethod = "csa" // client-side apply
+	req.NoError(rollAction.cfg.Releases.Create(rel2))
+
+	// Set empty description (should use default)
+	rollAction.Description = ""
+	rollAction.Version = 1
+	rollAction.ServerSideApply = "false" // Disable server-side apply for testing
+
+	err := rollAction.Run("test-release-empty")
+	req.NoError(err)
+
+	// Get the new release (version 3)
+	newReleasei, err := rollAction.cfg.Releases.Get("test-release-empty", 3)
+	req.NoError(err)
+	newRelease, err := releaserToV1Release(newReleasei)
+	req.NoError(err)
+
+	// Verify the default description was used for empty string
+	is.Equal("Rollback to 1", newRelease.Info.Description)
+}
+
+func TestRollback_DescriptionTooLong(t *testing.T) {
+	req := require.New(t)
+
+	rollAction := rollbackAction(t)
+
+	rel1 := releaseStub()
+	rel1.Name = "test-release-desc-long"
+	rel1.Version = 1
+	rel1.Info.Status = common.StatusSuperseded
+	rel1.ApplyMethod = "csa"
+	req.NoError(rollAction.cfg.Releases.Create(rel1))
+
+	rel2 := releaseStub()
+	rel2.Name = "test-release-desc-long"
+	rel2.Version = 2
+	rel2.Info.Status = common.StatusDeployed
+	rel2.ApplyMethod = "csa"
+	req.NoError(rollAction.cfg.Releases.Create(rel2))
+
+	rollAction.Description = strings.Repeat("a", MaxDescriptionLength+1)
+	rollAction.Version = 1
+	rollAction.ServerSideApply = "false"
+
+	err := rollAction.Run("test-release-desc-long")
+	req.Error(err)
+	req.Contains(err.Error(), "description must be")
+}
+
+func TestRollback_DescriptionAtMaxLength(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	rollAction := rollbackAction(t)
+
+	rel1 := releaseStub()
+	rel1.Name = "test-release-desc-max"
+	rel1.Version = 1
+	rel1.Info.Status = common.StatusSuperseded
+	rel1.ApplyMethod = "csa"
+	req.NoError(rollAction.cfg.Releases.Create(rel1))
+
+	rel2 := releaseStub()
+	rel2.Name = "test-release-desc-max"
+	rel2.Version = 2
+	rel2.Info.Status = common.StatusDeployed
+	rel2.ApplyMethod = "csa"
+	req.NoError(rollAction.cfg.Releases.Create(rel2))
+
+	rollAction.Description = strings.Repeat("a", MaxDescriptionLength)
+	rollAction.Version = 1
+	rollAction.ServerSideApply = "false"
+
+	err := rollAction.Run("test-release-desc-max")
+	req.NoError(err)
+
+	newReleasei, err := rollAction.cfg.Releases.Get("test-release-desc-max", 3)
+	req.NoError(err)
+	newRelease, err := releaserToV1Release(newReleasei)
+	req.NoError(err)
+
+	is.Equal(strings.Repeat("a", MaxDescriptionLength), newRelease.Info.Description)
+}
+
+func TestRollback_DescriptionMultiByteCharacters(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	rollAction := rollbackAction(t)
+
+	rel1 := releaseStub()
+	rel1.Name = "test-release-desc-utf8"
+	rel1.Version = 1
+	rel1.Info.Status = common.StatusSuperseded
+	rel1.ApplyMethod = "csa"
+	req.NoError(rollAction.cfg.Releases.Create(rel1))
+
+	rel2 := releaseStub()
+	rel2.Name = "test-release-desc-utf8"
+	rel2.Version = 2
+	rel2.Info.Status = common.StatusDeployed
+	rel2.ApplyMethod = "csa"
+	req.NoError(rollAction.cfg.Releases.Create(rel2))
+
+	// "é" is 2 bytes in UTF-8 but 1 rune
+	rollAction.Description = strings.Repeat("é", MaxDescriptionLength)
+	rollAction.Version = 1
+	rollAction.ServerSideApply = "false"
+
+	err := rollAction.Run("test-release-desc-utf8")
+	req.NoError(err)
+
+	newReleasei, err := rollAction.cfg.Releases.Get("test-release-desc-utf8", 3)
+	req.NoError(err)
+	newRelease, err := releaserToV1Release(newReleasei)
+	req.NoError(err)
+
+	is.Equal(strings.Repeat("é", MaxDescriptionLength), newRelease.Info.Description)
+}

pkg/cmd/rollback.go

@@ -21,6 +21,7 @@ import (
 	"io"
 	"strconv"
 	"time"
+	"unicode/utf8"
 
 	"github.com/spf13/cobra"
 
@@ -66,6 +67,11 @@ func newRollbackCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 				client.Version = ver
 			}
 
+			// Validate description length
+			if descLen := utf8.RuneCountInString(client.Description); descLen > action.MaxDescriptionLength {
+				return fmt.Errorf("description must be %d characters or less, got %d", action.MaxDescriptionLength, descLen)
+			}
+
 			dryRunStrategy, err := cmdGetDryRunFlagStrategy(cmd, false)
 			if err != nil {
 				return err
@@ -82,6 +88,7 @@ func newRollbackCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	}
 
 	f := cmd.Flags()
+	f.StringVar(&client.Description, "description", "", fmt.Sprintf("add a custom description for the rollback (max %d characters)", action.MaxDescriptionLength))
 	f.BoolVar(&client.ForceReplace, "force-replace", false, "force resource updates by replacement")
 	f.BoolVar(&client.ForceReplace, "force", false, "deprecated")
 	f.MarkDeprecated("force", "use --force-replace instead")

pkg/cmd/rollback_test.go

@@ -19,8 +19,10 @@ package cmd
 import (
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 
+	"helm.sh/helm/v4/pkg/action"
 	chart "helm.sh/helm/v4/pkg/chart/v2"
 	"helm.sh/helm/v4/pkg/release/common"
 	release "helm.sh/helm/v4/pkg/release/v1"
@@ -79,6 +81,11 @@ func TestRollbackCmd(t *testing.T) {
 		golden:    "output/rollback-no-args.txt",
 		rels:      rels,
 		wantError: true,
+	}, {
+		name:   "rollback a release with description",
+		cmd:    "rollback funny-honey 1 --description 'Reverting due to bug in version 2'",
+		golden: "output/rollback.txt",
+		rels:   rels,
 	}}
 	runTestCmd(t, tests)
 }
@@ -125,6 +132,83 @@ func TestRollbackFileCompletion(t *testing.T) {
 	checkFileCompletion(t, "rollback myrelease 1", false)
 }
 
+func TestRollbackWithDescription(t *testing.T) {
+	releaseName := "funny-bunny-desc"
+	rels := []*release.Release{
+		{
+			Name:    releaseName,
+			Info:    &release.Info{Status: common.StatusSuperseded},
+			Chart:   &chart.Chart{},
+			Version: 1,
+		},
+		{
+			Name:    releaseName,
+			Info:    &release.Info{Status: common.StatusDeployed},
+			Chart:   &chart.Chart{},
+			Version: 2,
+		},
+	}
+	storage := storageFixture()
+	for _, rel := range rels {
+		if err := storage.Create(rel); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	customDescription := "Rollback due to critical bug in version 2"
+	_, _, err := executeActionCommandC(storage, fmt.Sprintf("rollback %s 1 --description '%s'", releaseName, customDescription))
+	if err != nil {
+		t.Fatalf("unexpected error, got '%v'", err)
+	}
+
+	// Verify the description was stored correctly
+	updatedReli, err := storage.Get(releaseName, 3)
+	if err != nil {
+		t.Fatalf("unexpected error getting release, got '%v'", err)
+	}
+	updatedRel, err := releaserToV1Release(updatedReli)
+	if err != nil {
+		t.Fatalf("unexpected error converting release, got '%v'", err)
+	}
+
+	if updatedRel.Info.Description != customDescription {
+		t.Errorf("Expected description '%s', got '%s'", customDescription, updatedRel.Info.Description)
+	}
+}
+
+func TestRollbackDescriptionTooLong(t *testing.T) {
+	releaseName := "funny-bunny-long-desc"
+	rels := []*release.Release{
+		{
+			Name:    releaseName,
+			Info:    &release.Info{Status: common.StatusSuperseded},
+			Chart:   &chart.Chart{},
+			Version: 1,
+		},
+		{
+			Name:    releaseName,
+			Info:    &release.Info{Status: common.StatusDeployed},
+			Chart:   &chart.Chart{},
+			Version: 2,
+		},
+	}
+	storage := storageFixture()
+	for _, rel := range rels {
+		if err := storage.Create(rel); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	longDescription := strings.Repeat("a", action.MaxDescriptionLength+1)
+	_, _, err := executeActionCommandC(storage, fmt.Sprintf("rollback %s 1 --description '%s'", releaseName, longDescription))
+	if err == nil {
+		t.Error("expected error for description exceeding max length, got success")
+	}
+	if err != nil && !strings.Contains(err.Error(), fmt.Sprintf("description must be %d characters or less", action.MaxDescriptionLength)) {
+		t.Errorf("expected error about description length, got: %v", err)
+	}
+}
+
 func TestRollbackWithLabels(t *testing.T) {
 	labels1 := map[string]string{"operation": "install", "firstLabel": "firstValue"}
 	labels2 := map[string]string{"operation": "upgrade", "secondLabel": "secondValue"}