Made changes to the methods New and newUnchecked (removed bundleRepo …

…argument) (vmware-tanzu#316)

Removed BundleDownloader argument from getSupportedRegistry as it was not used
Made changes to installer tests and cli to work with the changes.
Made changes to agent/reconciler/host_reconciler.go so tests run
Updated the path where the bundle is contained to have the bundle-registry in the path.
Made install and uninstall to set the bundleRepo
Changed all required things so it can run with those changes.
Made unit test to check if download dir is correctly created and renamed after successful download.
Squashed all commits into one.

agent/installer/bundle_downloader.go

@@ -50,7 +50,10 @@ func (bd *bundleDownloader) DownloadFromRepo(
 	tag string,
 	downloadByTool func(string, string) error) error {
 
-	err := ensureDirExist(bd.downloadPath)
+	downloadPathWithRepo := bd.getBundlePathWithRepo()
+
+	err := ensureDirExist(downloadPathWithRepo)
+	defer os.Remove(downloadPathWithRepo)
 	if err != nil {
 		return err
 	}
@@ -65,7 +68,7 @@ func (bd *bundleDownloader) DownloadFromRepo(
 
 	bd.logger.Info("Cache miss", "path", bundleDirPath)
 
-	dir, err := os.MkdirTemp(bd.downloadPath, "tempBundle")
+	dir, err := os.MkdirTemp(downloadPathWithRepo, "tempBundle")
 	// It is fine if the dir path does not exist.
 	defer func() {
 		err = os.RemoveAll(dir)
@@ -76,7 +79,6 @@ func (bd *bundleDownloader) DownloadFromRepo(
 	if err != nil {
 		return err
 	}
-
 	bundleAddr := bd.getBundleAddr(normalizedOsVersion, k8sVersion, tag)
 	err = convertError(downloadByTool(bundleAddr, dir))
 	if err != nil {
@@ -125,14 +127,19 @@ func (bd *bundleDownloader) GetBundleDirPath(k8sVersion, tag string) string {
 	// Not storing tag as a subdir of k8s because we can't atomically move
 	// the temp bundle dir to a non-existing dir.
 	// Using "-" instead of ":" because Windows doesn't like the latter
-	return fmt.Sprintf("%s-%s", filepath.Join(bd.downloadPath, k8sVersion), tag)
+	return fmt.Sprintf("%s-%s", filepath.Join(bd.getBundlePathWithRepo(), k8sVersion), tag)
 }
 
 // GetBundleName returns the name of the bundle in normalized format.
 func GetBundleName(normalizedOsVersion, k8sVersion string) string {
 	return strings.ToLower(fmt.Sprintf("byoh-bundle-%s_k8s_%s", normalizedOsVersion, k8sVersion))
 }
 
+// getBundlePathWithRepo returns the path
+func (bd *bundleDownloader) getBundlePathWithRepo() string {
+	return filepath.Join(bd.downloadPath, strings.ReplaceAll(bd.repoAddr, "/", "."))
+}
+
 // getBundleAddr returns the exact address to the bundle in the repo.
 func (bd *bundleDownloader) getBundleAddr(normalizedOsVersion, k8sVersion, tag string) string {
 	return fmt.Sprintf("%s/%s:%s", bd.repoAddr, GetBundleName(normalizedOsVersion, k8sVersion), tag)

agent/installer/bundle_downloader_test.go

@@ -84,6 +84,25 @@ var _ = Describe("Byohost Installer Tests", func() {
 				mi.Get)
 			Expect(err).ShouldNot(HaveOccurred())
 		})
+		It("Should create and rename dir correctly after successful download", func() {
+			bd.repoAddr = "repo.ccoomm/r/"
+			bd.downloadPath = filepath.Join(bd.downloadPath)
+			err := bd.DownloadFromRepo(
+				normalizedOsVersion,
+				k8sVersion,
+				testTag,
+				mi.Get)
+			Expect(err).ShouldNot((HaveOccurred()))
+			_, err = os.Stat(bd.GetBundleDirPath(k8sVersion, testTag))
+			Expect(err).ShouldNot((HaveOccurred()))
+			notExist := os.IsNotExist(err)
+			Expect(notExist).ShouldNot(BeTrue())
+
+			_, err = os.Stat(bd.GetBundleDirPath(k8sVersion+"a", testTag))
+			Expect(err).Should((HaveOccurred()))
+			notExist = os.IsNotExist(err)
+			Expect(notExist).Should(BeTrue())
+		})
 	})
 	Context("When there is error during download", func() {
 		It("Should return error if given bad repo", func() {

agent/installer/cli.go

@@ -130,23 +130,22 @@ func runInstaller(install bool) {
 	var err error
 	if *osFlag != "" {
 		// Override current OS detection
-		i, err = newUnchecked(*osFlag, *bundleRepoFlag, *cachePathFlag, klogger, &logPrinter{klogger})
+		i, err = newUnchecked(*osFlag, *cachePathFlag, klogger, &logPrinter{klogger})
 		if err != nil {
 			klogger.Error(err, "unable to create installer")
 			return
 		}
 	} else {
-		i, err = New(*bundleRepoFlag, *cachePathFlag, klogger)
+		i, err = New(*cachePathFlag, klogger)
 		if err != nil {
 			klogger.Error(err, "unable to create installer")
 			return
 		}
 	}
-
 	if install {
-		err = i.Install(*k8sFlag, *tagFlag)
+		err = i.Install(*bundleRepoFlag, *k8sFlag, *tagFlag)
 	} else {
-		err = i.Uninstall(*k8sFlag, *tagFlag)
+		err = i.Uninstall(*bundleRepoFlag, *k8sFlag, *tagFlag)
 	}
 	if err != nil {
 		klogger.Error(err, "error installing/uninstalling")

agent/installer/installer.go

@@ -32,7 +32,7 @@ type installer struct {
 }
 
 // getSupportedRegistry returns a registry with installers for the supported OS and K8s
-func getSupportedRegistry(bd *bundleDownloader, ob algo.OutputBuilder) registry {
+func getSupportedRegistry(ob algo.OutputBuilder) registry {
 	reg := newRegistry()
 
 	addBundleInstaller := func(osBundle, k8sBundle string, stepProvider algo.K8sStepProvider) {
@@ -85,13 +85,10 @@ func (bd *bundleDownloader) DownloadOrPreview(os, k8s, tag string) error {
 	return bd.Download(os, k8s, tag)
 }
 
-// New returns an installer that downloads bundles for the current OS from OCI repository with
-// address bundleRepo and stores them under downloadPath. Download path is created,
+// New returns an installer that downloads bundles for the current OS
+// and stores them under downloadPath. Download path is created,
 // if it does not exist.
-func New(bundleRepo, downloadPath string, logger logr.Logger) (*installer, error) {
-	if bundleRepo == "" {
-		return nil, fmt.Errorf("empty bundle repo")
-	}
+func New(downloadPath string, logger logr.Logger) (*installer, error) {
 	if downloadPath == "" {
 		return nil, fmt.Errorf("empty download path")
 	}
@@ -103,16 +100,16 @@ func New(bundleRepo, downloadPath string, logger logr.Logger) (*installer, error
 		return nil, ErrDetectOs
 	}
 
-	return newUnchecked(os, bundleRepo, downloadPath, logger, &logPrinter{logger})
+	return newUnchecked(os, downloadPath, logger, &logPrinter{logger})
 }
 
-// newUnchecked returns an installer bypassing os detection and checks of bundleRepo and downloadPath.
-// If they are empty, returned installer will run in preview mode, i.e.
+// newUnchecked returns an installer bypassing os detection and checks of downloadPath.
+// If it is empty, returned installer will run in preview mode, i.e.
 // executes everything except the actual commands.
-func newUnchecked(currentOs, bundleRepo, downloadPath string, logger logr.Logger, outputBuilder algo.OutputBuilder) (*installer, error) {
-	bd := bundleDownloader{repoAddr: bundleRepo, downloadPath: downloadPath, logger: logger}
+func newUnchecked(currentOs, downloadPath string, logger logr.Logger, outputBuilder algo.OutputBuilder) (*installer, error) {
+	bd := bundleDownloader{repoAddr: "", downloadPath: downloadPath, logger: logger}
 
-	reg := getSupportedRegistry(&bd, outputBuilder)
+	reg := getSupportedRegistry(outputBuilder)
 	if len(reg.ListK8s(currentOs)) == 0 {
 		return nil, ErrOsK8sNotSupported
 	}
@@ -124,8 +121,14 @@ func newUnchecked(currentOs, bundleRepo, downloadPath string, logger logr.Logger
 		logger:           logger}, nil
 }
 
+// setBundleRepo sets the repo from which the bundle will be downloaded.
+func (i *installer) setBundleRepo(bundleRepo string) {
+	i.bundleDownloader.repoAddr = bundleRepo
+}
+
 // Install installs the specified k8s version on the current OS
-func (i *installer) Install(k8sVer, tag string) error {
+func (i *installer) Install(bundleRepo, k8sVer, tag string) error {
+	i.setBundleRepo(bundleRepo)
 	algoInst, err := i.getAlgoInstallerWithBundle(k8sVer, tag)
 	if err != nil {
 		return err
@@ -138,8 +141,9 @@ func (i *installer) Install(k8sVer, tag string) error {
 	return nil
 }
 
-// Uninstall uninstalls the specified k8s version on the current OS
-func (i *installer) Uninstall(k8sVer, tag string) error {
+// Uninstal uninstalls the specified k8s version on the current OS
+func (i *installer) Uninstall(bundleRepo, k8sVer, tag string) error {
+	i.setBundleRepo(bundleRepo)
 	algoInst, err := i.getAlgoInstallerWithBundle(k8sVer, tag)
 	if err != nil {
 		return err
@@ -191,15 +195,15 @@ func ListSupportedK8s(os string) []string {
 // getSupportedRegistryDescription returns a description registry of supported OS and k8s.
 // It that can only be queried for OS and k8s but cannot be used for install/uninstall.
 func getSupportedRegistryDescription() registry {
-	return getSupportedRegistry(nil, nil)
+	return getSupportedRegistry(nil)
 }
 
 // PreviewChanges describes the changes to install and uninstall K8s on OS without actually applying them.
 // It returns install and uninstall changes
 // Can be invoked on a non-supported OS
 func PreviewChanges(os, k8sVer string) (install, uninstall string, err error) {
 	stepPreviewer := stringPrinter{msgFmt: "# %s"}
-	reg := getSupportedRegistry(&bundleDownloader{}, &stepPreviewer)
+	reg := getSupportedRegistry(&stepPreviewer)
 	installer, _ := reg.GetInstaller(os, k8sVer)
 
 	if installer == nil {

agent/installer/installer_test.go

@@ -16,20 +16,14 @@ var _ = Describe("Byohost Installer Tests", func() {
 
 	Context("When installer is created for unsupported OS", func() {
 		It("Should return error", func() {
-			_, err := newUnchecked("Ubuntu_99.04.3_x86-64", "", "", logr.Discard(), nil)
-			Expect(err).Should(HaveOccurred())
-		})
-	})
-	Context("When installer is created with empty bundle repo", func() {
-		It("Should return error", func() {
-			_, err := New("", "downloadPath", logr.Discard())
-			Expect(err).Should(HaveOccurred())
+			_, err := newUnchecked("Ubuntu_99.04.3_x86-64", "", logr.Discard(), nil)
+			Expect(err).Should((HaveOccurred()))
 		})
 	})
 	Context("When installer is created with empty download path", func() {
 		It("Should return error", func() {
-			_, err := New("repo", "", logr.Discard())
-			Expect(err).Should(HaveOccurred())
+			_, err := New("", logr.Discard())
+			Expect(err).Should((HaveOccurred()))
 		})
 	})
 	Context("When installer is created", func() {
@@ -38,11 +32,11 @@ var _ = Describe("Byohost Installer Tests", func() {
 			for _, os := range osList {
 				i := NewPreviewInstaller(os, nil)
 
-				err := i.Install("unsupported-k8s", testTag)
-				Expect(err).Should(HaveOccurred())
+				err := i.Install("", "unsupported-k8s", testTag)
+				Expect(err).Should((HaveOccurred()))
 
-				err = i.Uninstall("unsupported-k8s", testTag)
-				Expect(err).Should(HaveOccurred())
+				err = i.Uninstall("", "unsupported-k8s", testTag)
+				Expect(err).Should((HaveOccurred()))
 			}
 		})
 	})
@@ -54,16 +48,16 @@ var _ = Describe("Byohost Installer Tests", func() {
 					{
 						ob := algo.OutputBuilderCounter{}
 						i := NewPreviewInstaller(os, &ob)
-						err := i.Install(k8s, testTag)
-						Expect(err).ShouldNot(HaveOccurred())
+						err := i.Install("", k8s, testTag)
+						Expect(err).ShouldNot((HaveOccurred()))
 						Expect(ob.LogCalledCnt).Should(Equal(22))
 					}
 
 					{
 						ob := algo.OutputBuilderCounter{}
 						i := NewPreviewInstaller(os, &ob)
-						err := i.Uninstall(k8s, testTag)
-						Expect(err).ShouldNot(HaveOccurred())
+						err := i.Uninstall("", k8s, testTag)
+						Expect(err).ShouldNot((HaveOccurred()))
 						Expect(ob.LogCalledCnt).Should(Equal(22))
 					}
 				}
@@ -131,7 +125,7 @@ var _ = Describe("Byohost Installer Tests", func() {
 })
 
 func NewPreviewInstaller(os string, ob algo.OutputBuilder) *installer {
-	i, err := newUnchecked(os, "", "", logr.Discard(), ob)
+	i, err := newUnchecked(os, "", logr.Discard(), ob)
 	if err != nil {
 		panic(err)
 	}

agent/reconciler/host_reconciler.go

@@ -254,11 +254,11 @@ func (r *HostReconciler) installK8sComponents(ctx context.Context, byoHost *infr
 	bundleRegistry := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupBaseRegistryAnnotation]
 	k8sVersion := byoHost.GetAnnotations()[infrastructurev1beta1.K8sVersionAnnotation]
 	byohBundleTag := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupTagAnnotation]
-	bundleInstaller, err := installer.New(bundleRegistry, r.DownloadPath, logger)
+	bundleInstaller, err := installer.New(r.DownloadPath, logger)
 	if err != nil {
 		return err
 	}
-	err = bundleInstaller.Install(k8sVersion, byohBundleTag)
+	err = bundleInstaller.Install(bundleRegistry, k8sVersion, byohBundleTag)
 	if err != nil {
 		return err
 	}
@@ -274,11 +274,11 @@ func (r *HostReconciler) uninstallk8sComponents(ctx context.Context, byoHost *in
 	bundleRegistry := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupBaseRegistryAnnotation]
 	k8sVersion := byoHost.GetAnnotations()[infrastructurev1beta1.K8sVersionAnnotation]
 	byohBundleTag := byoHost.GetAnnotations()[infrastructurev1beta1.BundleLookupTagAnnotation]
-	bundleInstaller, err := installer.New(bundleRegistry, r.DownloadPath, logger)
+	bundleInstaller, err := installer.New(r.DownloadPath, logger)
 	if err != nil {
 		return err
 	}
-	err = bundleInstaller.Uninstall(k8sVersion, byohBundleTag)
+	err = bundleInstaller.Uninstall(bundleRegistry, k8sVersion, byohBundleTag)
 	if err != nil {
 		return err
 	}