Apply suggestions from code review

Co-authored-by: Camila Macedo <[email protected]>

Author: vitorfloriano

pkg/cli/alpha/internal/update.go

@@ -11,6 +11,7 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/afero"
+	"sigs.k8s.io/kubebuilder/v4/pkg/config/store"
 	"sigs.k8s.io/kubebuilder/v4/pkg/config/store/yaml"
 	"sigs.k8s.io/kubebuilder/v4/pkg/machinery"
 )
@@ -20,6 +21,8 @@ type Update struct {
 	// FromVersion specifies which version of Kubebuilder to use for the update.
 	// If empty, the version from the PROJECT file will be used.
 	FromVersion string
+	// CliVersion holds the version to be used during the upgrade process
+	CliVersion string
 }
 
 // Update performs a complete project update by creating a three-way merge to help users
@@ -29,31 +32,21 @@ type Update struct {
 // - upgrade: New Kubebuilder version scaffolding
 // - merge: Attempts to merge upgrade changes into current state
 func (opts *Update) Update() error {
-	// Load the PROJECT configuration file to get the current CLI version
-	projectConfigFile := yaml.New(machinery.Filesystem{FS: afero.NewOsFs()})
-	if err := projectConfigFile.LoadFrom(yaml.DefaultPath); err != nil { // TODO: assess if DefaultPath could be renamed to a more self-descriptive name
-		return fmt.Errorf("fail to run command: %w", err)
-	}
+	// Load the PROJECT configuration file
+	projectConfigFile, _ := opts.loadConfigFile()
 
-	// Determine which Kubebuilder version to use for the update
-	cliVersion := projectConfigFile.Config().GetCliVersion()
+	// Extract the cliVersion field from the PROJECT file
+	opts.CliVersion = projectConfigFile.Config().GetCliVersion()
 
-	// Allow override of the version from PROJECT file via command line flag
-	if opts.FromVersion != "" {
-		// Ensure version has 'v' prefix for consistency with GitHub releases
-		if !strings.HasPrefix(opts.FromVersion, "v") {
-			opts.FromVersion = "v" + opts.FromVersion
-		}
-		log.Infof("Overriding cliVersion field %s from PROJECT file with --from-version %s", cliVersion, opts.FromVersion)
-		cliVersion = opts.FromVersion
-	} else {
-		log.Infof("Using CLI version from PROJECT file: %s", cliVersion)
+	// Determine which Kubebuilder version to use for the update
+	if err := opts.defineFromVersion(); err != nil {
+		return fmt.Errorf("failed to define version: %w", err)
 	}
 
 	// Download the specific Kubebuilder binary version for generating clean scaffolding
-	tempDir, err := opts.downloadKubebuilderBinary(cliVersion)
+	tempDir, err := opts.downloadKubebuilderBinary()
 	if err != nil {
-		return fmt.Errorf("failed to download Kubebuilder %s binary: %w", cliVersion, err)
+		return fmt.Errorf("failed to download Kubebuilder %s binary: %w", opts.CliVersion, err)
 	}
 	log.Infof("Downloaded binary kept at %s for debugging purposes", tempDir)
 
@@ -68,7 +61,7 @@ func (opts *Update) Update() error {
 	}
 
 	// Generate clean scaffolding using the old Kubebuilder version
-	if err := opts.runAlphaGenerate(tempDir, cliVersion); err != nil {
+	if err := opts.runAlphaGenerate(tempDir, opts.CliVersion); err != nil {
 		return fmt.Errorf("failed to run alpha generate on ancestor branch: %w", err)
 	}
 
@@ -95,21 +88,42 @@ func (opts *Update) Update() error {
 	return nil
 }
 
+// Load the PROJECT configuration file to get the current CLI version
+func (opts *Update) loadConfigFile() (store.Store, error) {
+	projectConfigFile := yaml.New(machinery.Filesystem{FS: afero.NewOsFs()})
+	// TODO: assess if DefaultPath could be renamed to a more self-descriptive name
+	if err := projectConfigFile.LoadFrom(yaml.DefaultPath); err != nil {
+		return projectConfigFile, fmt.Errorf("fail to run command: %w", err)
+	}
+	return projectConfigFile, nil
+}
+
+// Define the version of the binary to be downloaded
+func (opts *Update) defineFromVersion() error {
+	// Allow override of the version from PROJECT file via command line flag
+	if opts.FromVersion != "" {
+		// Ensure version has 'v' prefix for consistency with GitHub releases
+		if !strings.HasPrefix(opts.FromVersion, "v") {
+			opts.FromVersion = "v" + opts.FromVersion
+		}
+		opts.CliVersion = opts.FromVersion
+	}
+	return nil
+}
+
 // downloadKubebuilderBinary downloads the specified version of Kubebuilder binary
 // from GitHub releases and saves it to a temporary directory with executable permissions.
 // Returns the temporary directory path containing the binary.
-func (opts *Update) downloadKubebuilderBinary(version string) (string, error) {
-	cliVersion := version
-
+func (opts *Update) downloadKubebuilderBinary() (string, error) {
 	// Construct GitHub release URL based on current OS and architecture
 	url := fmt.Sprintf("https://github.com/kubernetes-sigs/kubebuilder/releases/download/%s/kubebuilder_%s_%s",
-		cliVersion, runtime.GOOS, runtime.GOARCH)
+		opts.CliVersion, runtime.GOOS, runtime.GOARCH)
 
-	log.Infof("Downloading the Kubebuilder %s binary from: %s", cliVersion, url)
+	log.Infof("Downloading the Kubebuilder %s binary from: %s", opts.CliVersion, url)
 
 	// Create temporary directory for storing the downloaded binary
 	fs := afero.NewOsFs()
-	tempDir, err := afero.TempDir(fs, "", "kubebuilder"+cliVersion+"-")
+	tempDir, err := afero.TempDir(fs, "", "kubebuilder"+opts.CliVersion+"-")
 	if err != nil {
 		return "", fmt.Errorf("failed to create temporary directory: %w", err)
 	}
@@ -145,7 +159,7 @@ func (opts *Update) downloadKubebuilderBinary(version string) (string, error) {
 		return "", fmt.Errorf("failed to make binary executable: %w", err)
 	}
 
-	log.Infof("Kubebuilder version %s succesfully downloaded to %s", cliVersion, binaryPath)
+	log.Infof("Kubebuilder version %s successfully downloaded to %s", opts.CliVersion, binaryPath)
 
 	return tempDir, nil
 }
@@ -195,19 +209,27 @@ func (opts *Update) cleanUpAncestorBranch() error {
 // to create clean scaffolding in the ancestor branch. This uses the downloaded
 // binary with the original PROJECT file to recreate the project's initial state.
 func (opts *Update) runAlphaGenerate(tempDir, version string) error {
-	tempBinaryPath := tempDir + "/kubebuilder"
+	// Restore the original PROJECT file from master branch to ensure
+	// we're using the correct project configuration for scaffolding
+	gitCmd := exec.Command("git", "checkout", "master", "--", "PROJECT")
+	if err := gitCmd.Run(); err != nil {
+		return fmt.Errorf("failed to checkout PROJECT from master")
+	}
+	log.Info("Successfully checked out the PROJECT file from master branch")
 
 	// Temporarily modify PATH to use the downloaded Kubebuilder binary
+	tempBinaryPath := tempDir + "/kubebuilder"
 	originalPath := os.Getenv("PATH")
 	tempEnvPath := tempDir + ":" + originalPath
 
 	if err := os.Setenv("PATH", tempEnvPath); err != nil {
 		return fmt.Errorf("failed to set temporary PATH: %w", err)
 	}
+
 	// Restore original PATH when function completes
 	defer func() {
 		if err := os.Setenv("PATH", originalPath); err != nil {
-			log.Errorf("failed to restore original PATH: %w", err)
+			log.Errorf("failed to restore original PATH: %v", err)
 		}
 	}()
 
@@ -219,11 +241,11 @@ func (opts *Update) runAlphaGenerate(tempDir, version string) error {
 
 	// Restore the original PROJECT file from master branch to ensure
 	// we're using the correct project configuration for scaffolding
-	gitCmd := exec.Command("git", "checkout", "master", "--", "PROJECT")
+	gitCmd = exec.Command("git", "checkout", "master", "--", "PROJECT")
 	if err := gitCmd.Run(); err != nil {
 		return fmt.Errorf("failed to checkout PROJECT from master")
 	}
-	log.Info("Succesfully checked out the PROJECT file from master branch")
+	log.Info("Successfully checked out the PROJECT file from master branch")
 
 	// Execute the alpha generate command to create clean scaffolding
 	if err := cmd.Run(); err != nil {
@@ -243,7 +265,7 @@ func (opts *Update) runAlphaGenerate(tempDir, version string) error {
 	if err := gitCmd.Run(); err != nil {
 		return fmt.Errorf("failed to commit changes in ancestor: %w", err)
 	}
-	log.Info("Successfully commited changes in ancestor")
+	log.Info("Successfully committed changes in ancestor")
 
 	return nil
 }
@@ -278,7 +300,7 @@ func (opts *Update) checkoutCurrentOffAncestor() error {
 	if err := gitCmd.Run(); err != nil {
 		return fmt.Errorf("failed to commit changes: %w", err)
 	}
-	log.Info("Successfully commited changes in current")
+	log.Info("Successfully committed changes in current")
 
 	return nil
 }
@@ -317,7 +339,7 @@ func (opts *Update) checkoutUpgradeOffAncestor() error {
 	if err := gitCmd.Run(); err != nil {
 		return fmt.Errorf("failed to commit changes in upgrade branch: %w", err)
 	}
-	log.Info("Successfully commited changes in upgrade branch")
+	log.Info("Successfully committed changes in upgrade branch")
 
 	return nil
 }

pkg/cli/alpha/update.go

@@ -20,33 +20,30 @@ func NewUpdateCommand() *cobra.Command {
 	updateCmd := &cobra.Command{
 		Use:   "update",
 		Short: "Update a Kubebuilder project to a newer version",
-		Long: `Update a Kubebuilder project to a newer version using a three-way merge strategy.
+		Long: `This command upgrades your Kubebuilder project to the latest scaffold layout using a 3-way merge strategy.
 
-This command helps you upgrade your Kubebuilder project by:
-1. Creating a clean ancestor branch with the old version's scaffolding
-2. Creating a current branch with your project's current state
-3. Creating an upgrade branch with the new version's scaffolding
-4. Attempting to merge the changes automatically
+It performs the following steps:
+  1. Creates an 'ancestor' branch from the version originally used to scaffold the project
+  2. Creates a 'current' branch with your project's current state
+  3. Creates an 'upgrade' branch using the new version's scaffolding
+  4. Attempts a 3-way merge into a 'merge' branch
 
-The process creates several Git branches to help you manage the upgrade:
-- ancestor: Clean scaffolding from the original version
-- current: Your project's current state
-- upgrade: Clean scaffolding from the new version
-- merge: Attempted automatic merge of upgrade into current
+The process uses Git branches:
+  - ancestor: clean scaffold from the original version
+  - current: your existing project state
+  - upgrade: scaffold from the target version
+  - merge: result of the 3-way merge
 
-If conflicts occur during the merge, you'll need to resolve them manually.
+If conflicts occur during the merge, resolve them manually in the 'merge' branch. 
+Once resolved, commit and push it as a pull request. This branch will contain the 
+final upgraded project with the latest Kubebuilder layout and your custom code.
 
 Examples:
   # Update using the version specified in PROJECT file
   kubebuilder alpha update
 
   # Update from a specific version
-  kubebuilder alpha update --from-version v3.0.0
-
-Requirements:
-- Must be run from the root of a Kubebuilder project
-- Git repository must be clean (no uncommitted changes)
-- PROJECT file must exist and contain a valid layout version`,
+  kubebuilder alpha update --from-version v3.0.0`,
 
 		// TODO: Add validation to ensure we're in a Kubebuilder project and Git repo is clean
 		//	PreRunE: func(_ *cobra.Command, _ []string) error {
@@ -62,7 +59,7 @@ Requirements:
 
 	// Flag to override the version specified in the PROJECT file
 	updateCmd.Flags().StringVar(&opts.FromVersion, "from-version", "",
-		"Override the CLI version from PROJECT file. Specify the Kubebuilder version to upgrade from (e.g., 'v3.0.0' or '3.0.0')")
+		"Kubebuilder binary release version to upgrade from. Should match the version used to init the project.")
 
 	return updateCmd
 }