refactor: address review comments

Signed-off-by: Akash Kumar <[email protected]>

pkg/client/client.go

@@ -313,6 +313,23 @@ func (c *KpmClient) ResolvePkgDepsMetadata(kclPkg *pkg.KclPkg, update bool) erro
 	return nil
 }
 
+func GetReleasesFromSource(sourceType, uri string) ([]string, error) {
+	var releases []string
+	var err error
+
+	switch sourceType {
+	case pkg.GIT:
+		releases, err = git.GetAllGithubReleases(uri)
+	case pkg.OCI:
+		releases, err = oci.GetAllImageTags(uri)
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	return releases, nil
+}
+
 // UpdateDeps will update the dependencies.
 func (c *KpmClient) UpdateDeps(kclPkg *pkg.KclPkg) error {
 	_, err := c.ResolveDepsMetadataInJsonStr(kclPkg, true)

pkg/client/client_test.go

@@ -10,10 +10,12 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
+	"sort"
 	"strings"
 	"testing"
 
 	"github.com/dominikbraun/graph"
+	"github.com/hashicorp/go-version"
 	"github.com/otiai10/copy"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/mod/module"
@@ -815,6 +817,43 @@ func TestParseOciOptionFromString(t *testing.T) {
 	assert.Equal(t, ociOption.Tag, "test_tag")
 }
 
+func TestGetReleasesFromSource(t *testing.T) {
+	sortVersions := func(versions []string) ([]string, error) {
+		var vers []*version.Version
+		for _, raw := range versions {
+			v, err := version.NewVersion(raw)
+			if err != nil {
+				return nil, err
+			}
+			vers = append(vers, v)
+		}
+		sort.Slice(vers, func(i, j int) bool {
+			return vers[i].LessThan(vers[j])
+		})
+		var res []string
+		for _, v := range vers {
+			res = append(res, v.Original())
+		}
+		return res, nil
+	}
+
+	releases, err := GetReleasesFromSource(pkg.GIT, "https://github.com/kcl-lang/kpm")
+	assert.Equal(t, err, nil)
+	length := len(releases)
+	assert.True(t, length >= 5)
+	releasesVersions, err := sortVersions(releases)
+	assert.Equal(t, err, nil)
+	assert.Equal(t, releasesVersions[:5], []string{"v0.1.0", "v0.2.0", "v0.2.1", "v0.2.2", "v0.2.3"})
+
+	releases, err = GetReleasesFromSource(pkg.OCI, "oci://ghcr.io/kcl-lang/k8s")
+	assert.Equal(t, err, nil)
+	length = len(releases)
+	assert.True(t, length >= 5)
+	releasesVersions, err = sortVersions(releases)
+	assert.Equal(t, err, nil)
+	assert.Equal(t, releasesVersions[:5], []string{"1.14", "1.15", "1.16", "1.17", "1.18"})
+}
+
 func TestUpdateWithKclMod(t *testing.T) {
 	kpmcli, err := NewKpmClient()
 	assert.Equal(t, err, nil)
@@ -1469,4 +1508,3 @@ func testRunWithOciDownloader(t *testing.T) {
 	assert.Equal(t, buf.String(), "downloading 'zong-zhe/helloworld:0.0.3' from 'ghcr.io/zong-zhe/helloworld:0.0.3'\n")
 	assert.Equal(t, res.GetRawYamlResult(), "The_first_kcl_program: Hello World!")
 }
-

pkg/cmd/cmd_add.go

@@ -208,7 +208,7 @@ func parseGitRegistryOptions(c *cli.Context) (*opt.RegistryOptions, *reporter.Kp
 // parseOciRegistryOptions will parse the oci registry information from user cli inputs.
 func parseOciRegistryOptions(c *cli.Context, kpmcli *client.KpmClient) (*opt.RegistryOptions, error) {
 	ociPkgRef := c.Args().First()
-	name, version, err := parseOciPkgNameAndVersion(ociPkgRef)
+	name, version, err := ParseOciPkgNameAndVersion(ociPkgRef)
 	if err != nil {
 		return nil, err
 	}
@@ -243,7 +243,7 @@ func parseLocalPathOptions(c *cli.Context) (*opt.RegistryOptions, *reporter.KpmE
 
 // parseOciPkgNameAndVersion will parse package name and version
 // from string "<pkg_name>:<pkg_version>".
-func parseOciPkgNameAndVersion(s string) (string, string, error) {
+func ParseOciPkgNameAndVersion(s string) (string, string, error) {
 	parts := strings.Split(s, ":")
 	if len(parts) == 1 {
 		return parts[0], "", nil

pkg/cmd/cmd_update.go

@@ -13,7 +13,6 @@ import (
 	"github.com/dominikbraun/graph"
 	"github.com/urfave/cli/v2"
 	"golang.org/x/mod/module"
-	mvsExt "kcl-lang.io/kpm/pkg/3rdparty/mvs"
 	"kcl-lang.io/kpm/pkg/client"
 	"kcl-lang.io/kpm/pkg/env"
 	"kcl-lang.io/kpm/pkg/mvs"
@@ -64,14 +63,6 @@ func KpmUpdate(c *cli.Context, kpmcli *client.KpmClient) error {
 		return reporter.NewErrorEvent(reporter.Bug, err, "internal bugs, please contact us to fix it.")
 	}
 
-	fetchNameAndVersion := func(pkgInfo string) (string, string) {
-		// the version is specified after :
-		if strings.Contains(pkgInfo, ":") {
-			return strings.Split(pkgInfo, ":")[0], strings.Split(pkgInfo, ":")[1]
-		}
-		return pkgInfo, ""
-	}
-
 	kclPkg, err := kpmcli.LoadPkgFromPath(pwd)
 	if err != nil {
 		return err
@@ -92,47 +83,10 @@ func KpmUpdate(c *cli.Context, kpmcli *client.KpmClient) error {
 		modulesToDowngrade []module.Version
 	)
 
-	// validate if the packages is present in kcl.mod file and
-	// find the latest version if version is not specified
 	for _, pkgInfo := range pkgInfos {
-		pkgInfo = strings.TrimSpace(pkgInfo)
-		pkgName, pkgVersion := fetchNameAndVersion(pkgInfo)
-
-		var dep pkg.Dependency
-		var ok bool
-		if dep, ok = kclPkg.Deps[pkgName]; !ok {
-			reporter.Report(fmt.Sprintf("package %s not found in kcl.mod file", pkgName))
-			continue
-		}
-
-		if pkgVersion == "" {
-			var releases []string
-			if dep.Git != nil {
-				releases, err = mvs.GetReleasesFromSource(dep.GetSourceType(), dep.Git.Url)
-			} else if dep.Oci != nil {
-				releases, err = mvs.GetReleasesFromSource(dep.GetSourceType(), "fjdkasd")
-			}
-			if err != nil {
-				reporter.ReportEventToStdout(reporter.NewEvent(
-					reporter.FailedGetReleases,
-					fmt.Sprintf("failed to get releases for %s", pkgName),
-				))
-				continue
-			}
-			pkgVersion, err = semver.LatestCompatibleVersion(releases, dep.Version)
-			if err != nil {
-				reporter.NewErrorEvent(
-					reporter.FailedSelectLatestCompatibleVersion,
-					err,
-					fmt.Sprintf("failed to find the latest version for %s", pkgName),
-				)
-				continue
-			}
-		}
-		if pkgVersion < dep.Version {
-			modulesToDowngrade = append(modulesToDowngrade, module.Version{Path: pkgName, Version: pkgVersion})
-		} else if pkgVersion > dep.Version {
-			modulesToUpgrade = append(modulesToUpgrade, module.Version{Path: pkgName, Version: pkgVersion})
+		err = GetModulesToUpdate(kclPkg, modulesToUpgrade, modulesToDowngrade, pkgInfo)
+		if err != nil {
+			reporter.Report(err)
 		}
 	}
 
@@ -148,11 +102,12 @@ func KpmUpdate(c *cli.Context, kpmcli *client.KpmClient) error {
 	}
 
 	target := module.Version{Path: kclPkg.GetPkgName(), Version: kclPkg.GetPkgVersion()}
-	buildList, err := UpdateBuildList(target, modulesToUpgrade, modulesToDowngrade, &reqs)
+	buildList, err := mvs.UpdateBuildList(target, modulesToUpgrade, modulesToDowngrade, &reqs)
 	if err != nil {
 		return reporter.NewErrorEvent(reporter.FailedUpdatingBuildList, err, "failed to update build list")
 	}
 
+	// get all the vertices in the graph 
 	modules, err := graph.TopologicalSort(depGraph)
 	if err != nil {
 		return reporter.NewErrorEvent(reporter.FailedTopologicalSort, err, "failed to sort the dependencies")
@@ -161,29 +116,10 @@ func KpmUpdate(c *cli.Context, kpmcli *client.KpmClient) error {
 	kclPkg.ModFile.Dependencies.Deps = make(map[string]pkg.Dependency)
 
 	for _, module := range modules {
-		if module.Path == target.Path || !slices.Contains(buildList, module) {
-			continue
-		}
-		d := pkg.Dependency{
-			Name:    module.Path,
-			Version: module.Version,
-		}
-		d.FullName = d.GenDepFullName()
-		_, properties, err := depGraph.VertexWithProperties(module)
+		err = InsertModuleToDeps(kclPkg, module, target, buildList, reqs)
 		if err != nil {
-			return reporter.NewErrorEvent(reporter.FailedGetVertexProperties, err, "failed to get vertex with properties")
-		}
-		// there must be one property depending on the download source type
-		for sourceType, uri := range properties.Attributes {
-			d.Source, err = pkg.GenSource(sourceType, uri, module.Version)
-			if err != nil {
-				return reporter.NewErrorEvent(reporter.FailedGenerateSource, err, "failed to generate source")
-			}
-			// if d.Oci != nil && d.Oci.Reg == "ghcr.io" {
-			// 	d.Oci = nil
-			// }
+			return err
 		}
-		kclPkg.ModFile.Dependencies.Deps[module.Path] = d
 	}
 
 	err = kpmcli.UpdateDeps(kclPkg)
@@ -193,27 +129,71 @@ func KpmUpdate(c *cli.Context, kpmcli *client.KpmClient) error {
 	return nil
 }
 
-func UpdateBuildList(target module.Version, modulesToUpgrade []module.Version, modulesToDowngrade []module.Version, reqs *mvs.ReqsGraph) ([]module.Version, error) {
-	var (
-		UpdBuildLists []module.Version
-		err           error
-	)
+// GetModulesToUpdate validates if the packages is present in kcl.mod file and
+// find the latest version if version is not specified. Depending on the value of pkgVersion,
+// modulesToUpgrade or modulesToDowngrade will be updated. 
+func GetModulesToUpdate(kclPkg *pkg.KclPkg, modulesToUpgrade []module.Version,  modulesToDowngrade []module.Version, pkgInfo string) error {
+	pkgInfo = strings.TrimSpace(pkgInfo)
+	pkgName, pkgVersion, err := ParseOciPkgNameAndVersion(pkgInfo)
+	if err != nil {
+		return err
+	}
+
+	var dep pkg.Dependency
+	var ok bool
+	if dep, ok = kclPkg.Deps[pkgName]; !ok {
+		return err
+	}
 
-	if len(modulesToUpgrade) == 0 {
-		UpdBuildLists, err = mvsExt.UpgradeAll(target, reqs)
-	} else {
-		UpdBuildLists, err = mvsExt.Upgrade(target, reqs, modulesToUpgrade...)
+	if pkgVersion == "" {
+		var releases []string
+		releases, err = client.GetReleasesFromSource(dep.GetSourceType(), dep.GetDownloadPath())
+		if err != nil {
+			return reporter.NewErrorEvent(
+				reporter.FailedGetReleases,
+				err,
+				fmt.Sprintf("failed to get releases for %s", pkgName),
+			)
+		}
+		pkgVersion, err = semver.LatestCompatibleVersion(releases, dep.Version)
+		if err != nil {
+			return reporter.NewErrorEvent(
+				reporter.FailedSelectLatestCompatibleVersion,
+				err,
+				fmt.Sprintf("failed to find the latest version for %s", pkgName),
+			)
+		}
 	}
-	if err != nil {
-		return []module.Version{}, err
+	if pkgVersion < dep.Version {
+		modulesToDowngrade = append(modulesToDowngrade, module.Version{Path: pkgName, Version: pkgVersion})
+	} else if pkgVersion > dep.Version {
+		modulesToUpgrade = append(modulesToUpgrade, module.Version{Path: pkgName, Version: pkgVersion})
 	}
+	return nil
+}
 
-	if len(modulesToDowngrade) != 0 {
-		UpdBuildLists, err = mvsExt.Downgrade(target, reqs, modulesToDowngrade...)
+// InsertModuleToDeps checks whether module is present in the buildList and it is not the same as the target module, 
+// and inserts it to the dependencies of kclPkg
+func InsertModuleToDeps(kclPkg *pkg.KclPkg, module module.Version, target module.Version, buildList []module.Version, reqs mvs.ReqsGraph) (error) {
+	if module.Path == target.Path || !slices.Contains(buildList, module) {
+		return nil
+	}
+	d := pkg.Dependency{
+		Name:    module.Path,
+		Version: module.Version,
 	}
+	d.FullName = d.GenDepFullName()
+	_, properties, err := reqs.VertexWithProperties(module)
 	if err != nil {
-		return []module.Version{}, err
+		return reporter.NewErrorEvent(reporter.FailedGetVertexProperties, err, "failed to get vertex with properties")
 	}
-
-	return UpdBuildLists, nil
-}
+	// there must be one property depending on the download source type
+	for sourceType, uri := range properties.Attributes {
+		d.Source, err = pkg.GenSource(sourceType, uri, module.Version)
+		if err != nil {
+			return reporter.NewErrorEvent(reporter.FailedGenerateSource, err, "failed to generate source")
+		}
+	}
+	kclPkg.ModFile.Dependencies.Deps[module.Path] = d
+	return nil
+}

pkg/mvs/mvs.go

@@ -6,10 +6,9 @@ import (
 	"github.com/dominikbraun/graph"
 	"github.com/hashicorp/go-version"
 	"golang.org/x/mod/module"
+	"kcl-lang.io/kpm/pkg/3rdparty/mvs"
 	"kcl-lang.io/kpm/pkg/client"
 	errInt "kcl-lang.io/kpm/pkg/errors"
-	"kcl-lang.io/kpm/pkg/git"
-	"kcl-lang.io/kpm/pkg/oci"
 	pkg "kcl-lang.io/kpm/pkg/package"
 	"kcl-lang.io/kpm/pkg/reporter"
 	"kcl-lang.io/kpm/pkg/semver"
@@ -55,9 +54,9 @@ func (r ReqsGraph) Upgrade(m module.Version) (module.Version, error) {
 		return module.Version{}, errInt.MultipleSources
 	}
 
-	var releases []string 
+	var releases []string
 	for sourceType, uri := range properties.Attributes {
-		releases, err = GetReleasesFromSource(sourceType, uri)
+		releases, err = client.GetReleasesFromSource(sourceType, uri)
 		if err != nil {
 			return module.Version{}, err
 		}
@@ -113,7 +112,7 @@ func (r ReqsGraph) Previous(m module.Version) (module.Version, error) {
 
 	var releases []string
 	for sourceType, uri := range properties.Attributes {
-		releases, err = GetReleasesFromSource(sourceType, uri)
+		releases, err = client.GetReleasesFromSource(sourceType, uri)
 		if err != nil {
 			return module.Version{}, err
 		}
@@ -176,19 +175,32 @@ func (r ReqsGraph) Required(m module.Version) ([]module.Version, error) {
 	return reqs, nil
 }
 
-func GetReleasesFromSource(sourceType, uri string) ([]string, error) {
-	var releases []string
-	var err error
+// UpdateBuildList decides whether to upgrade or downgrade based on modulesToUpgrade and modulesToDowngrade.
+// if modulesToUpgrade is empty, upgrade all dependencies. if modulesToUpgrade is not empty, upgrade the dependencies.
+// if modulesToDowngrade is not empty, downgrade the dependencies.
+// if modulesToUpgrade and modulesToDowngrade are both empty, first apply upgrade operation and
+// then downgrade the build list returned from previous operation.
+func UpdateBuildList(target module.Version, modulesToUpgrade []module.Version, modulesToDowngrade []module.Version, reqs *ReqsGraph) ([]module.Version, error) {
+	var (
+		UpdBuildLists []module.Version
+		err           error
+	)
+
+	if len(modulesToUpgrade) == 0 {
+		UpdBuildLists, err = mvs.UpgradeAll(target, reqs)
+	} else {
+		UpdBuildLists, err = mvs.Upgrade(target, reqs, modulesToUpgrade...)
+	}
+	if err != nil {
+		return []module.Version{}, err
+	}
 
-	switch sourceType {
-	case pkg.GIT:
-		releases, err = git.GetAllGithubReleases(uri)
-	case pkg.OCI:
-		releases, err = oci.GetAllImageTags(uri)
+	if len(modulesToDowngrade) != 0 {
+		UpdBuildLists, err = mvs.Downgrade(target, reqs, modulesToDowngrade...)
 	}
 	if err != nil {
-		return nil, err
+		return []module.Version{}, err
 	}
 
-	return releases, nil
-}
+	return UpdBuildLists, nil
+}