refactored pkg/lib/kptops by asmb123 · Pull Request #4394 · kptdev/kpt

asmb123 · 2026-02-17T03:12:40Z

Refactored pkg/lib/kptops (as mentioned in issue #4379) :

Implemented helper functions for the PkgUpdate function and their tests
error string literals in a constants.go
Removed commented code

Signed-off-by: asmb123 <asmit.barua.phe23@itbhu.ac.in>

netlify · 2026-02-17T03:12:45Z

✅ Deploy Preview for kptdocs ready!

Name	Link
🔨 Latest commit	`d6d3f4f`
🔍 Latest deploy log	https://app.netlify.com/projects/kptdocs/deploys/6993dcabb43aca00083a2aad
😎 Deploy Preview	https://deploy-preview-4394--kptdocs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

Refactors parts of pkg/lib/kptops related to package update/clone flows (issue #4379), aiming to improve structure, consolidate error messages, and add unit tests around new helper functionality.

Changes:

Added helper functions around PkgUpdate (fetching repos, merge strategy selection/application, lockfile saving, OCI fetch stub) and introduced new tests.
Centralized several clone-related error message strings into constants.go.
Renamed git normalization helpers and updated clone tests accordingly; removed commented-out code in PkgUpdate.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
pkg/lib/kptops/pkgupdate.go	Removes commented code and adds multiple helper functions for update flow; adjusts logging.
pkg/lib/kptops/pkgupdate_test.go	Adds new unit tests for update helpers (strategy selection, OCI fetch stub, error wrapper, constants).
pkg/lib/kptops/errors.go	Adds `wrapError` helper for consistent error wrapping.
pkg/lib/kptops/constants.go	Introduces string constants for clone-related error messages.
pkg/lib/kptops/clone.go	Switches to error message constants; renames git normalization helpers.
pkg/lib/kptops/clone_test.go	Updates tests to use renamed normalization helper functions.

Comments suppressed due to low confidence (1)

pkg/lib/kptops/pkgupdate.go:98

The cleanup defers use os.RemoveAll(updated.AbsPath()) / originRepoSpec.AbsPath(), but AbsPath() is filepath.Join(RepoSpec.Dir, RepoSpec.Path). When Path is non-empty (common for packages in subdirectories), this will delete only the package subdir and leave the temp clone directory (RepoSpec.Dir) behind, leaking temp dirs. Use updated.Dir / originRepoSpec.Dir for cleanup (or return a cleanup func from the fetch helper).

		defer os.RemoveAll(updated.AbsPath())
		updatedDir = updated.AbsPath()
		updatedRepoSpec = updated

		// var origin repoClone
		if kf.UpstreamLock != nil {
			gLock := kf.UpstreamLock.Git
			originRepoSpec := &git.RepoSpec{OrgRepo: gLock.Repo, Path: gLock.Directory, Ref: gLock.Commit}
			klog.Infof("Fetching origin from %s@%s", originRepoSpec.OrgRepo, originRepoSpec.Ref)
			if err := fetch.NewCloner(originRepoSpec).ClonerUsingGitExec(ctx); err != nil {
				return err
			}
			originDir = originRepoSpec.AbsPath()
		} else {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-17T03:17:44Z

pkg/lib/kptops/pkgupdate_test.go

+	// Verify the returned path exists and is within tmpDir
+	if !strings.HasPrefix(path, tmpDir) {
+		t.Errorf("FetchOCI() path %s not within tmpDir %s", path, tmpDir)
+	}


TestFetchOCI checks that path is within tmpDir using strings.HasPrefix, which is brittle for filesystem paths (path separators/case, and prefix collisions). Prefer a path-aware check (e.g., filepath.Rel and ensure the result doesn’t start with ..).

Copilot · 2026-02-17T03:17:45Z

pkg/lib/kptops/pkgupdate_test.go

+// TestUpdateSubpackages tests subpackage recursion (placeholder).
+func TestUpdateSubpackages(t *testing.T) {
+	opts := &PkgUpdateOpts{Strategy: "ResourceMerge"}
+	err := updateSubpackages(".", ".", "./updated", "./origin", opts)
+	if err == nil {
+		// For now, this is a placeholder implementation
+		t.Logf("updateSubpackages() completed (placeholder implementation)")
+	}
+}
+


TestUpdateSubpackages doesn’t assert anything: it passes even when updateSubpackages returns an error, which can mask regressions and adds noise to the suite. Either implement a deterministic test using temp dirs/fixtures and assert the expected outcome, or remove this placeholder until the feature is implemented.

Suggested change

// TestUpdateSubpackages tests subpackage recursion (placeholder).

func TestUpdateSubpackages(t *testing.T) {

opts := &PkgUpdateOpts{Strategy: "ResourceMerge"}

err := updateSubpackages(".", ".", "./updated", "./origin", opts)

if err == nil {

// For now, this is a placeholder implementation

t.Logf("updateSubpackages() completed (placeholder implementation)")

}

}

Copilot · 2026-02-17T03:17:45Z

pkg/lib/kptops/pkgupdate.go

@@ -182,19 +126,180 @@ func PkgUpdate(ctx context.Context, ref string, packageDir string, _ PkgUpdateOp
 		if err := updater.Update(updateOptions); err != nil {
 			return err


PkgUpdate still hard-codes ResourceMergeUpdater and ignores the merge strategy in PkgUpdateOpts. The helper functions added below (validateInput, selectMergeStrategy, ApplyMerge, SaveLockFile) are not used, so callers cannot actually choose a strategy and the refactor is incomplete. Wire PkgUpdate to accept and use the provided opts (don’t discard it), validate inputs, select the updater via selectMergeStrategy, and run the merge through the shared helper(s).

Copilot · 2026-02-17T03:17:45Z

pkg/lib/kptops/pkgupdate.go

+	updated := *upstream
+	if err := fetch.NewCloner(&updated).ClonerUsingGitExec(ctx); err != nil {
+		return "", "", nil, wrapError("fetch upstream", err)
+	}
+	updatedDir := updated.AbsPath()
+
+	var originDir string
+	if kf.UpstreamLock != nil {
+		gLock := kf.UpstreamLock.Git
+		originRepoSpec := &git.RepoSpec{OrgRepo: gLock.Repo, Path: gLock.Directory, Ref: gLock.Commit}
+		klog.Infof("Fetching origin from %s@%s", originRepoSpec.OrgRepo, originRepoSpec.Ref)
+
+		if err := fetch.NewCloner(originRepoSpec).ClonerUsingGitExec(ctx); err != nil {
+			return "", "", nil, wrapError("fetch origin", err)
+		}
+		originDir = originRepoSpec.AbsPath()
+	} else {
+		dir, err := os.MkdirTemp("", "kpt-empty-")
+		if err != nil {
+			return "", "", nil, wrapError("create temp directory for origin", err)
+		}
+		originDir = dir
+	}

+	return updatedDir, originDir, &updated, nil


FetchRepositories clones repos to disk but doesn’t clean up the resulting directories (e.g., updated.AbsPath() and the temp origin dir when UpstreamLock is nil). If callers forget to remove these, it will leak temp directories over time. Consider either handling cleanup inside this helper (return a cleanup func) or clearly documenting that the caller must defer os.RemoveAll(...) for both returned dirs.

Copilot · 2026-02-17T03:17:45Z

pkg/lib/kptops/constants.go

+package kptops
+
+// Error message constants for clone operations
+const (


This new file is missing the standard Apache 2.0 license/copyright header that the other Go files in this package include. Add the same header block for consistency and to satisfy typical repository/license checks.

Copilot · 2026-02-17T03:17:46Z

pkg/lib/kptops/pkgupdate.go

+	return nil
+}
+
+// fetchRepositories fetches both the upstream and origin repositories based on the Kptfile.


The comment doesn’t match the exported function name (FetchRepositories). Go lint expects exported identifiers to have comments that begin with the identifier name. Update the comment to start with “FetchRepositories …” (and similarly for other exported helpers added in this file).

Suggested change

// fetchRepositories fetches both the upstream and origin repositories based on the Kptfile.

// FetchRepositories fetches both the upstream and origin repositories based on the Kptfile.

Copilot · 2026-02-17T03:17:46Z

pkg/lib/kptops/pkgupdate.go

+// selectMergeStrategy returns the appropriate Updater based on the Strategy field.
+func selectMergeStrategy(strategy string) (interface{}, error) {
+	switch strategy {
+	case "ResourceMerge", "":
+		return &update.ResourceMergeUpdater{}, nil
+	case "FastForward":
+		// return &update.FastForwardUpdater{}, nil
+		return nil, fmt.Errorf("FastForward strategy not yet supported")
+	case "ForceDeleteReplace":
+		// return &update.ForceDeleteReplaceUpdater{}, nil
+		return nil, fmt.Errorf("ForceDeleteReplace strategy not yet supported")
+	case "CopyMerge":
+		// return &update.CopyMergeUpdater{}, nil
+		return nil, fmt.Errorf("CopyMerge strategy not yet supported")
+	default:
+		return nil, fmt.Errorf("unsupported merge strategy: %s", strategy)
+	}
+}
+
+// applyMerge executes the merge operation with the selected strategy.
+func ApplyMerge(updater interface{}, updateOptions *updatetypes.Options) error {
+	if u, ok := updater.(*update.ResourceMergeUpdater); ok {
+		return wrapError("apply merge", u.Update(*updateOptions))
+	}
+	return fmt.Errorf("updater type not supported")
+}


selectMergeStrategy/ApplyMerge use interface{} and then type-assert to *update.ResourceMergeUpdater, which defeats the purpose of selecting a strategy and is easy to misuse. Prefer returning updatetypes.Updater from selectMergeStrategy and have ApplyMerge accept that interface and simply call Update; then add additional updater implementations as they become supported.

Copilot · 2026-02-17T03:17:46Z

pkg/lib/kptops/pkgupdate.go

+// applyMerge executes the merge operation with the selected strategy.
+func ApplyMerge(updater interface{}, updateOptions *updatetypes.Options) error {
+	if u, ok := updater.(*update.ResourceMergeUpdater); ok {
+		return wrapError("apply merge", u.Update(*updateOptions))
+	}
+	return fmt.Errorf("updater type not supported")
+}


The comment doesn’t match the exported function name (ApplyMerge). For exported functions, update the comment to start with “ApplyMerge …” to satisfy Go lint and keep docs consistent.

Copilot · 2026-02-17T03:17:46Z

pkg/lib/kptops/pkgupdate.go

+// saveLockFile updates the Kptfile.lock with the new upstream metadata.
+func SaveLockFile(packageDir string, updatedRepoSpec *git.RepoSpec) error {
+	if err := kptfileutil.UpdateUpstreamLockFromGit(packageDir, updatedRepoSpec); err != nil {
+		return wrapError("save lock file", err)
+	}
+	return nil
+}


The comment doesn’t match the exported function name (SaveLockFile). Update it to start with “SaveLockFile …” (exported identifier comment convention).

Copilot · 2026-02-17T03:17:47Z

pkg/lib/kptops/pkgupdate.go

+	return nil
+}
+
+// fetchOCI fetches an OCI package reference.


The comment doesn’t match the exported function name (FetchOCI). Update it to start with “FetchOCI …” (exported identifier comment convention).

Suggested change

// fetchOCI fetches an OCI package reference.

// FetchOCI fetches an OCI package reference.

refactored pkg/lib/kptops

d6d3f4f

Signed-off-by: asmb123 <asmit.barua.phe23@itbhu.ac.in>

Copilot AI review requested due to automatic review settings February 17, 2026 03:12

asmb123 requested review from efiacor, kispaljr, liamfallon and mozesl-nokia as code owners February 17, 2026 03:12

Copilot started reviewing on behalf of asmb123 February 17, 2026 03:13 View session

dosubot bot added area/pkg Testing labels Feb 17, 2026

Copilot AI reviewed Feb 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactored pkg/lib/kptops#4394

refactored pkg/lib/kptops#4394
asmb123 wants to merge 1 commit intokptdev:mainfrom
asmb123:main

asmb123 commented Feb 17, 2026

Uh oh!

netlify bot commented Feb 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		@@ -182,19 +126,180 @@ func PkgUpdate(ctx context.Context, ref string, packageDir string, _ PkgUpdateOp
		if err := updater.Update(updateOptions); err != nil {
		return err

	// fetchRepositories fetches both the upstream and origin repositories based on the Kptfile.
	// FetchRepositories fetches both the upstream and origin repositories based on the Kptfile.

	// fetchOCI fetches an OCI package reference.
	// FetchOCI fetches an OCI package reference.

Conversation

asmb123 commented Feb 17, 2026

Uh oh!

netlify bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kptdocs ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

netlify bot commented Feb 17, 2026 •

edited

Loading