package update: make version flag optional & remove call to GetPackag…

…e() (#768)

* package update: make version flag optional

Signed-off-by: Marjan Alavi <malavi@vmware.com>

cmd/cli/plugin/package/main.go

@@ -21,7 +21,6 @@ var descriptor = cliv1alpha1.PluginDescriptor{
 }
 
 var logLevel int32
-var logFile string
 var outputFormat string
 
 func main() {
@@ -31,7 +30,6 @@ func main() {
 	}
 
 	p.Cmd.PersistentFlags().Int32VarP(&logLevel, "verbose", "", 0, "Number for the log level verbosity(0-9)")
-	p.Cmd.PersistentFlags().StringVar(&logFile, "log-file", "", "Log file path")
 
 	p.AddCommands(
 		repositoryCmd,

cmd/cli/plugin/package/package_available_get.go

@@ -114,7 +114,7 @@ func packageAvailableGet(cmd *cobra.Command, args []string) error {
 
 func getValuesSchema(cmd *cobra.Command, args []string, kc kappclient.Client) error {
 	if pkgVersion == "" {
-		return errors.New("version is required when values-schema flag is declared. Please specify <PACKAGE-NAME>/<VERSION>")
+		return errors.New("version is required when --values-schema flag is declared. Please specify <PACKAGE-NAME>/<VERSION>")
 	}
 	pkg, pkgGetErr := kc.GetPackage(fmt.Sprintf("%s.%s", pkgName, pkgVersion), packageAvailableOp.Namespace)
 	if pkgGetErr != nil {

cmd/cli/plugin/package/package_installed_update.go

@@ -6,6 +6,7 @@ package main
 import (
 	"fmt"
 
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 
 	"github.com/vmware-tanzu/tanzu-framework/pkg/v1/tkg/log"
@@ -14,7 +15,7 @@ import (
 )
 
 var packageInstalledUpdateCmd = &cobra.Command{
-	Use:   "update INSTALLED_PACKAGE_NAME --version VERSION",
+	Use:   "update INSTALLED_PACKAGE_NAME",
 	Short: "Update an installed package",
 	Args:  cobra.ExactArgs(1),
 	Example: `
@@ -24,21 +25,33 @@ var packageInstalledUpdateCmd = &cobra.Command{
 }
 
 func init() {
-	packageInstalledUpdateCmd.Flags().StringVarP(&packageInstalledOp.Version, "version", "v", "", "The version which installed package needs to be updated to")
-	packageInstalledUpdateCmd.Flags().StringVarP(&packageInstalledOp.ValuesFile, "values-file", "f", "", "The path to the configuration values file")
+	packageInstalledUpdateCmd.Flags().StringVarP(&packageInstalledOp.Version, "version", "v", "", "The version which installed package needs to be updated to, optional")
+	packageInstalledUpdateCmd.Flags().StringVarP(&packageInstalledOp.ValuesFile, "values-file", "f", "", "The path to the configuration values file, optional")
 	packageInstalledUpdateCmd.Flags().BoolVarP(&packageInstalledOp.Install, "install", "", false, "Install package if the installed package does not exist, optional")
 	packageInstalledUpdateCmd.Flags().StringVarP(&packageInstalledOp.PackageName, "package-name", "p", "", "The public name for the package, optional")
 	packageInstalledUpdateCmd.Flags().StringVarP(&packageInstalledOp.Namespace, "namespace", "n", "default", "The namespace to locate the installed package which needs to be updated")
 	packageInstalledUpdateCmd.Flags().BoolVarP(&packageInstalledOp.Wait, "wait", "", true, "Wait for the package reconciliation to complete, optional. To disable wait, specify --wait=false")
 	packageInstalledUpdateCmd.Flags().DurationVarP(&packageInstalledOp.PollInterval, "poll-interval", "", tkgpackagedatamodel.DefaultPollInterval, "Time interval between subsequent polls of package reconciliation status, optional")
 	packageInstalledUpdateCmd.Flags().DurationVarP(&packageInstalledOp.PollTimeout, "poll-timeout", "", tkgpackagedatamodel.DefaultPollTimeout, "Timeout value for polls of package reconciliation status, optional")
-	packageInstalledUpdateCmd.MarkFlagRequired("version") //nolint
 	packageInstalledCmd.AddCommand(packageInstalledUpdateCmd)
 }
 
 func packageUpdate(cmd *cobra.Command, args []string) error {
 	packageInstalledOp.PkgInstallName = args[0]
 
+	if packageInstalledOp.Version == "" && packageInstalledOp.ValuesFile == "" {
+		return errors.New("please provide --version and/or --values-file for updating the installed package")
+	}
+
+	if packageInstalledOp.Install {
+		if packageInstalledOp.PackageName == "" {
+			return errors.New("--package-name is required when --install flag is declared")
+		}
+		if packageInstalledOp.Version == "" {
+			return errors.New("--version is required when --install flag is declared")
+		}
+	}
+
 	cmd.SilenceUsage = true
 
 	pkgClient, err := tkgpackageclient.NewTKGPackageClient(packageInstalledOp.KubeConfig)

pkg/v1/tkg/tkgpackageclient/package_install.go

@@ -13,7 +13,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	k8serror "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	crtclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -51,7 +51,7 @@ func (p *pkgClient) InstallPackage(o *tkgpackagedatamodel.PackageOptions, progre
 	}()
 
 	if pkgInstall, err = p.kappClient.GetPackageInstall(o.PkgInstallName, o.Namespace); err != nil {
-		if !apierrors.IsNotFound(err) {
+		if !k8serror.IsNotFound(err) {
 			return
 		}
 		err = nil
@@ -62,20 +62,17 @@ func (p *pkgClient) InstallPackage(o *tkgpackagedatamodel.PackageOptions, progre
 		return
 	}
 
+	progress.ProgressMsg <- fmt.Sprintf("Getting package metadata for '%s'", o.PackageName)
+	if _, _, err = p.GetPackage(o); err != nil {
+		return
+	}
+
 	if o.CreateNamespace {
 		progress.ProgressMsg <- fmt.Sprintf("Creating namespace '%s'", o.Namespace)
 		if err = p.createNamespace(o.Namespace); err != nil {
 			return
 		}
-	} else {
-		progress.ProgressMsg <- fmt.Sprintf("Getting namespace '%s'", o.Namespace)
-		if err = p.kappClient.GetClient().Get(context.Background(), crtclient.ObjectKey{Name: o.Namespace}, &corev1.Namespace{}); err != nil {
-			return
-		}
-	}
-
-	progress.ProgressMsg <- fmt.Sprintf("Getting package metadata for '%s'", o.PackageName)
-	if _, _, err = p.GetPackage(o); err != nil {
+	} else if err = p.kappClient.GetClient().Get(context.Background(), crtclient.ObjectKey{Name: o.Namespace}, &corev1.Namespace{}); err != nil {
 		return
 	}
 
@@ -208,7 +205,7 @@ func (p *pkgClient) createNamespace(namespace string) error {
 		crtclient.ObjectKey{Name: namespace},
 		&corev1.Namespace{})
 	if err != nil {
-		if !apierrors.IsNotFound(err) {
+		if !k8serror.IsNotFound(err) {
 			return err
 		}
 		ns := &corev1.Namespace{
@@ -277,6 +274,7 @@ func (p *pkgClient) createServiceAccount(o *tkgpackagedatamodel.PackageOptions)
 // waitForResourceInstallation waits until the package get installed successfully or a failure happen
 func (p *pkgClient) waitForResourceInstallation(name, namespace string, pollInterval, pollTimeout time.Duration, progress chan string, rscType tkgpackagedatamodel.ResourceType) error {
 	var status kappctrl.GenericStatus
+	progress <- fmt.Sprintf("Waiting for '%s' reconciliation for '%s'", rscType.String(), name)
 	if err := wait.Poll(pollInterval, pollTimeout, func() (done bool, err error) {
 		switch rscType {
 		case tkgpackagedatamodel.ResourceTypePackageRepository:

pkg/v1/tkg/tkgpackageclient/package_install_test.go

@@ -184,7 +184,7 @@ var _ = Describe("Install Package", func() {
 		AfterEach(func() { options = opts })
 	})
 
-	Context("failure in getting installed package due to GetPackageInstall API error in waitForPackageInstallation", func() {
+	Context("failure in getting installed package due to GetPackageInstall API error in waitForResourceInstallation", func() {
 		BeforeEach(func() {
 			options.Wait = true
 			kappCtl = &fakes.KappClient{}

pkg/v1/tkg/tkgpackageclient/package_update.go

@@ -31,55 +31,54 @@ func (p *pkgClient) UpdatePackage(o *tkgpackagedatamodel.PackageOptions, progres
 	}()
 
 	progress.ProgressMsg <- fmt.Sprintf("Getting package install for '%s'", o.PkgInstallName)
-	pkgInstall, err = p.kappClient.GetPackageInstall(o.PkgInstallName, o.Namespace)
-	if err != nil {
-		if k8serror.IsNotFound(err) {
-			err = nil
-		} else {
+	if pkgInstall, err = p.kappClient.GetPackageInstall(o.PkgInstallName, o.Namespace); err != nil {
+		if !k8serror.IsNotFound(err) {
 			return
 		}
+		err = nil
 	}
 
 	if pkgInstall == nil {
 		if !o.Install {
 			err = &tkgpackagedatamodel.PackagePluginNonCriticalError{Reason: tkgpackagedatamodel.ErrPackageNotInstalled}
 			return
 		}
-		if o.PackageName == "" {
-			err = errors.New("package-name is required when install flag is declared")
-			return
-		}
 		progress.ProgressMsg <- fmt.Sprintf("Installing package '%s'", o.PkgInstallName)
 		p.InstallPackage(o, progress, tkgpackagedatamodel.OperationTypeUpdate)
 		return
 	}
 
 	pkgInstallToUpdate := pkgInstall.DeepCopy()
-	if o.Version != pkgInstallToUpdate.Status.Version {
-		if pkgInstallToUpdate.Spec.PackageRef == nil || pkgInstallToUpdate.Spec.PackageRef.VersionSelection == nil {
-			err = errors.New(fmt.Sprintf("failed to update package '%s'", o.PkgInstallName))
-			return
-		}
-		progress.ProgressMsg <- fmt.Sprintf("Getting package metadata for '%s'", pkgInstallToUpdate.Spec.PackageRef.RefName)
+
+	if pkgInstallToUpdate.Spec.PackageRef == nil || pkgInstallToUpdate.Spec.PackageRef.VersionSelection == nil {
+		err = errors.New(fmt.Sprintf("failed to update package '%s' as no existing package reference/version was found in the package install", o.PkgInstallName))
+		return
+	}
+
+	// If o.PackageName is provided by the user (via --package-name flag), set the package name in PackageInstall to it.
+	// This is useful for the case in which the user made a typo in the package-name at the time of installation and it failed and they want to fix it through package update.
+	// Otherwise if o.PackageName is not provided, fill it from the installed package spec, as the validation logic in GetPackage() needs this field to be set.
+	if o.PackageName != "" {
+		pkgInstallToUpdate.Spec.PackageRef.RefName = o.PackageName
+	} else {
 		o.PackageName = pkgInstallToUpdate.Spec.PackageRef.RefName
-		if _, _, err = p.GetPackage(o); err != nil {
-			return
-		}
+	}
+
+	// If o.Version is provided by the user (via --version flag), set the version in PackageInstall to this version
+	// Otherwise if o.Version is not provided, fill it from the installed package spec, as the validation logic in GetPackage() needs this field to be set.
+	if o.Version != "" {
 		pkgInstallToUpdate.Spec.PackageRef.VersionSelection.Constraints = o.Version
+	} else {
+		o.Version = pkgInstallToUpdate.Spec.PackageRef.VersionSelection.Constraints
 	}
 
-	if o.ValuesFile != "" {
-		if secretCreated, err = p.updateValuesFile(o, pkgInstallToUpdate, progress.ProgressMsg); err != nil {
-			return
-		}
+	progress.ProgressMsg <- fmt.Sprintf("Getting package metadata for '%s'", pkgInstallToUpdate.Spec.PackageRef.RefName)
+	if _, _, err = p.GetPackage(o); err != nil {
+		return
+	}
 
-		pkgInstallToUpdate.Spec.Values = []kappipkg.PackageInstallValues{
-			{
-				SecretRef: &kappipkg.PackageInstallValuesSecretRef{
-					Name: fmt.Sprintf(tkgpackagedatamodel.SecretName, o.PkgInstallName, o.Namespace),
-				},
-			},
-		}
+	if secretCreated, err = p.createOrUpdateValuesSecret(o, pkgInstallToUpdate, progress.ProgressMsg); err != nil {
+		return
 	}
 
 	progress.ProgressMsg <- fmt.Sprintf("Updating package install for '%s'", o.PkgInstallName)
@@ -95,13 +94,17 @@ func (p *pkgClient) UpdatePackage(o *tkgpackagedatamodel.PackageOptions, progres
 	}
 }
 
-// updateValuesFile either creates or updates the values secret depending on whether the corresponding annotation exist or not
-func (p *pkgClient) updateValuesFile(o *tkgpackagedatamodel.PackageOptions, pkgInstallToUpdate *kappipkg.PackageInstall, progress chan string) (bool, error) {
+// createOrUpdateValuesSecret either creates or updates the values secret depending on whether the corresponding annotation exists or not
+func (p *pkgClient) createOrUpdateValuesSecret(o *tkgpackagedatamodel.PackageOptions, pkgInstallToUpdate *kappipkg.PackageInstall, progress chan string) (bool, error) {
 	var (
 		secretCreated bool
 		err           error
 	)
 
+	if o.ValuesFile == "" {
+		return false, nil
+	}
+
 	o.SecretName = fmt.Sprintf(tkgpackagedatamodel.SecretName, o.PkgInstallName, o.Namespace)
 
 	if o.SecretName == pkgInstallToUpdate.GetAnnotations()[tkgpackagedatamodel.TanzuPkgPluginAnnotation+"-Secret"] {
@@ -117,6 +120,11 @@ func (p *pkgClient) updateValuesFile(o *tkgpackagedatamodel.PackageOptions, pkgI
 		}
 	}
 
+	pkgInstallToUpdate.Spec.Values = []kappipkg.PackageInstallValues{
+		{
+			SecretRef: &kappipkg.PackageInstallValuesSecretRef{Name: o.SecretName}},
+	}
+
 	return secretCreated, nil
 }