Skip to content

Fix incorrect handling when unfetched packages are deleted from upstream #4695

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aravindtga wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix incorrect handling when unfetched packages are deleted from upstream #4695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6308bf
Fix incorrect handling when unfetched packages are deleted from upstream
aravindtga Aug 12, 2026
2318011
Address review comments
aravindtga Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 100 additions & 0 deletions commands/pkg/update/cmdupdate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,6 +683,106 @@ Fetching origin from {{ (index .REPOS "foo").RepoDirectory }}@master
<git_output>
Updating package "subpkg1" with strategy "resource-merge".

Updated 2 package(s).
`,
},
"unfetched subpackage deleted from upstream": {
reposChanges: map[string][]testutil.Content{
testutil.Upstream: {
{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile().
WithResource(pkgbuilder.DeploymentResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", "master", "resource-merge"),
),
pkgbuilder.NewSubPkg("subpkg2").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", "master", "resource-merge"),
),
),
Branch: "master",
},
{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile().
WithResource(pkgbuilder.SecretResource),
},
},
"foo": {
{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile().
WithResource(pkgbuilder.DeploymentResource),
Branch: "master",
},
},
},
updatedLocal: testutil.Content{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef(testutil.Upstream, "/", "master", "resource-merge").
WithUpstreamLockRef(testutil.Upstream, "/", "master", 0),
).
WithResource(pkgbuilder.DeploymentResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", "master", "resource-merge").
WithUpstreamLockRef("foo", "/", "master", 0),
).
WithResource(pkgbuilder.DeploymentResource, pkgbuilder.SetFieldPath("5", "spec", "replicas")),
pkgbuilder.NewSubPkg("subpkg2").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", "master", "resource-merge").
WithUpstreamLockRef("foo", "/", "master", 0),
).
WithResource(pkgbuilder.DeploymentResource),
),
},
expectedLocal: pkgbuilder.NewRootPkg().
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef(testutil.Upstream, "/", "master", "resource-merge").
WithUpstreamLockRef(testutil.Upstream, "/", "master", 1),
).
WithResource(pkgbuilder.SecretResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", "master", "resource-merge").
WithUpstreamLockRef("foo", "/", "master", 0),
).
WithResource(pkgbuilder.DeploymentResource, pkgbuilder.SetFieldPath("5", "spec", "replicas")),
),
expectedOutput: `
Package "{{ .PKG_NAME }}":
Fetching upstream from {{ (index .REPOS "upstream").RepoDirectory }}@master
<git_output>
Fetching origin from {{ (index .REPOS "upstream").RepoDirectory }}@master
<git_output>
Updating package "{{ .PKG_NAME }}" with strategy "resource-merge".
<git_output>
Adding package "".
Deleting package "subpkg2" from local since it is removed in upstream.
<git_output>
Adding package "".
Package "subpkg1" deleted from upstream, but keeping local since it has changes.

Package "{{ .PKG_NAME }}/subpkg1":
Fetching upstream from {{ (index .REPOS "foo").RepoDirectory }}@master
<git_output>
Fetching origin from {{ (index .REPOS "foo").RepoDirectory }}@master
Updating package "subpkg1" with strategy "resource-merge".

Updated 2 package(s).
`,
},
Expand Down
43 changes: 43 additions & 0 deletions pkg/lib/update/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,49 @@ func (u Command) updatePackage(ctx context.Context, subPkgPath, localPath, updat

// Package deleted from upstream
case originExists && localExists && !updatedExists:
// If the package has been deleted from upstream, we only want to delete
// it from local if it has no changes.
originUnfetched, err := pkg.IsPackageUnfetched(originPath)
if err != nil {
return errors.E(op, kptfilev1.UniquePath(localPath), err)
}
// If the package in origin is in the unfetched state, we need to fetch
// it before we can diff with local. Otherwise the diff will always show
// changes since origin only contains the Kptfile while local has the
// full fetched content.
if originUnfetched {
// We want to fetch the package here, but we need to use the
// commit SHA from the package in local to make sure we get the correct
// ref from origin. So we read the Kptfile in local and use the
// commit SHA from there.
localKf, err := kptfileutil.ReadKptfile(filesys.FileSystemOrOnDisk{}, localPath)
if err != nil {
return errors.E(op, kptfilev1.UniquePath(localPath), err)
}
if localKf.UpstreamLock == nil || localKf.UpstreamLock.Git == nil {
return errors.E(op, kptfilev1.UniquePath(localPath),
fmt.Errorf("local package missing upstreamLock.git, cannot determine origin commit"))
}
localOriginSha := localKf.UpstreamLock.Git.Commit
p, err := pkg.New(filesys.FileSystemOrOnDisk{}, originPath)
if err != nil {
return errors.E(op, kptfilev1.UniquePath(localPath), err)
}
Comment thread
aravindtga marked this conversation as resolved.
// Fetch the package, but provide the correct commit SHA rather
// than just fetching based on the ref in the Kptfile. If the ref
// points to a branch, we don't want to end up getting a different
// ref than was used by local.
if err := (fetch.Command{
Pkg: p,
Commit: localOriginSha,
}).Run(ctx); err != nil {
Comment thread
aravindtga marked this conversation as resolved.
return errors.E(op, kptfilev1.UniquePath(localPath), err)
}
// Add merge comments to make sure we don't get unnecessary diffs.
if err := addmergecomment.Process(originPath); err != nil {
return errors.E(op, kptfilev1.UniquePath(localPath), err)
}
}
// Check the diff. If there are local changes, we keep the subpackage.
diff, err := copyutil.Diff(originPath, localPath)
if err != nil {
Expand Down
143 changes: 143 additions & 0 deletions pkg/lib/update/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3525,6 +3525,149 @@ func TestRun_remote_subpackages(t *testing.T) {
WithResource(pkgbuilder.ConfigMapResource),
),
},
"unfetched subpackage deleted from upstream with no local changes": {
reposChanges: map[string][]testutil.Content{
testutil.Upstream: {
{
Pkg: pkgbuilder.NewRootPkg().
WithResource(pkgbuilder.DeploymentResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge"),
),
pkgbuilder.NewSubPkg("subpkg2").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge"),
),
),
Branch: masterBranch,
},
{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile().
WithResource(pkgbuilder.ConfigMapResource),
},
},
"foo": {
{
Pkg: pkgbuilder.NewRootPkg().
WithResource(pkgbuilder.DeploymentResource),
Branch: masterBranch,
},
},
},
updatedLocal: testutil.Content{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef(testutil.Upstream, "/", masterBranch, "resource-merge").
WithUpstreamLockRef(testutil.Upstream, "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge").
WithUpstreamLockRef("foo", "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource),
pkgbuilder.NewSubPkg("subpkg2").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge").
WithUpstreamLockRef("foo", "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource),
),
},
expectedLocal: pkgbuilder.NewRootPkg().
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef(testutil.Upstream, "/", masterBranch, "resource-merge").
WithUpstreamLockRef(testutil.Upstream, "/", masterBranch, 1),
).
WithResource(pkgbuilder.ConfigMapResource),
},
"unfetched subpackage deleted from upstream but local has changes": {
reposChanges: map[string][]testutil.Content{
testutil.Upstream: {
{
Pkg: pkgbuilder.NewRootPkg().
WithResource(pkgbuilder.DeploymentResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge"),
),
pkgbuilder.NewSubPkg("subpkg2").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge"),
),
),
Branch: masterBranch,
},
{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile().
WithResource(pkgbuilder.ConfigMapResource),
},
},
"foo": {
{
Pkg: pkgbuilder.NewRootPkg().
WithResource(pkgbuilder.DeploymentResource),
Branch: masterBranch,
},
},
},
updatedLocal: testutil.Content{
Pkg: pkgbuilder.NewRootPkg().
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef(testutil.Upstream, "/", masterBranch, "resource-merge").
WithUpstreamLockRef(testutil.Upstream, "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge").
WithUpstreamLockRef("foo", "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource, pkgbuilder.SetFieldPath("5", "spec", "replicas")),
pkgbuilder.NewSubPkg("subpkg2").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge").
WithUpstreamLockRef("foo", "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource),
),
},
expectedLocal: pkgbuilder.NewRootPkg().
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef(testutil.Upstream, "/", masterBranch, "resource-merge").
WithUpstreamLockRef(testutil.Upstream, "/", masterBranch, 1),
).
WithResource(pkgbuilder.ConfigMapResource).
WithSubPackages(
pkgbuilder.NewSubPkg("subpkg1").
WithKptfile(
pkgbuilder.NewKptfile().
WithUpstreamRef("foo", "/", masterBranch, "resource-merge").
WithUpstreamLockRef("foo", "/", masterBranch, 0),
).
WithResource(pkgbuilder.DeploymentResource, pkgbuilder.SetFieldPath("5", "spec", "replicas")),
),
},
}

for tn, tc := range testCases {
Expand Down
54 changes: 38 additions & 16 deletions pkg/lib/util/fetch/fetch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,15 @@ import (
// provided package, and fetches the package referenced if it isn't already
// there.
type Command struct {
// Pkg is required and contains information about the package that should be
// fetched. The package directory referenced must contain a valid Kptfile.
Pkg *pkg.Pkg

// Commit stores a git commit SHA. If this is set when invoking the command,
// the package will be fetched based on this commit SHA rather than based on
// the ref provided in the Kptfile. This allows for fetching specific
// commits even if the Kptfile references a branch.
Commit string
}

// Run runs the Command.
Expand All @@ -58,6 +66,7 @@ func (c Command) Run(ctx context.Context) error {
OrgRepo: g.Repo,
Path: g.Directory,
Ref: g.Ref,
Commit: c.Commit,
}
err = NewCloner(repoSpec).cloneAndCopy(ctx, c.Pkg.UniquePath.String())
if err != nil {
Expand Down Expand Up @@ -176,21 +185,22 @@ func (c *Cloner) ClonerUsingGitExec(ctx context.Context) error {
c.cachedRepo[c.repoSpec.CloneSpec()] = upstreamRepo
}

// Check if we have a ref in the upstream that matches the package-specific
// reference. If we do, we use that reference.
ps := strings.Split(c.repoSpec.Path, "/")
for len(ps) != 0 {
p := path.Join(ps...)
packageRef := path.Join(strings.TrimLeft(p, "/"), c.repoSpec.Ref)
if _, found := upstreamRepo.ResolveTag(packageRef); found {
c.repoSpec.Ref = packageRef
break
}
ps = ps[:len(ps)-1]
var commit string
// If a commit SHA is provided in the repoSpec, we use it directly instead
// of resolving the ref. This allows fetching a specific commit even if the
// ref (e.g. a branch) has since moved forward.
if c.repoSpec.Commit != "" {
commit = c.repoSpec.Commit
} else {
// Check if we have a ref in the upstream that matches the package-specific
// reference. If we do, we use that reference.
ref := checkPackageTags(c.repoSpec.Path, c.repoSpec.Ref, upstreamRepo)
c.repoSpec.Ref = ref
commit = upstreamRepo.ResolveRef(ref)
}

// Pull the required ref into the repo git cache.
dir, err := upstreamRepo.GetRepo(ctx, []string{c.repoSpec.Ref})
dir, err := upstreamRepo.GetRepo(ctx, []string{commit})
Comment thread
aravindtga marked this conversation as resolved.
if err != nil {
return errors.E(op, errors.Git, errors.Repo(c.repoSpec.CloneSpec()), err)
}
Expand All @@ -200,10 +210,6 @@ func (c *Cloner) ClonerUsingGitExec(ctx context.Context) error {
return errors.E(op, errors.Git, errors.Repo(c.repoSpec.CloneSpec()), err)
}

// Find the commit SHA for the ref that was just fetched. We need the SHA
// rather than the ref to be able to do a hard reset of the cache repo.
commit := upstreamRepo.ResolveRef(c.repoSpec.Ref)

// Reset the local repo to the commit we need. Doing a hard reset instead of
// a checkout means we don't create any local branches so we don't need to
// worry about fast-forwarding them with changes from upstream. It also makes
Expand Down Expand Up @@ -289,3 +295,19 @@ func copyDir(ctx context.Context, srcDir string, dstDir string) error {
}
return copy.Copy(srcDir, dstDir, opts)
}

// checkPackageTags looks up the most specific tag for the package at the
// provided pkgPath. It checks if there is a tag in the upstream that matches
// a package-specific reference (i.e. path/ref).
func checkPackageTags(pkgPath, ref string, upstreamRepo internalgitutil.GitUpstreamRepo) string {
ps := strings.Split(pkgPath, "/")
for len(ps) != 0 {
p := path.Join(ps...)
packageRef := path.Join(strings.TrimLeft(p, "/"), ref)
if _, found := upstreamRepo.ResolveTag(packageRef); found {
return packageRef
}
ps = ps[:len(ps)-1]
}
return ref
}
Loading