From 5f000e0231bb525c8a4d293fc024483f6df0735b Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Fri, 11 Jun 2021 11:20:46 -0700 Subject: [PATCH 1/2] Add tests to verify the cli output for kpt pkg update --- internal/cmdupdate/cmdupdate_test.go | 365 ++++++++++++++++++++++++ internal/printer/fake/fake.go | 5 +- internal/testutil/pkgbuilder/builder.go | 11 +- internal/util/pkgutil/pkgutil.go | 9 +- 4 files changed, 379 insertions(+), 11 deletions(-) diff --git a/internal/cmdupdate/cmdupdate_test.go b/internal/cmdupdate/cmdupdate_test.go index 2a62611010..1ee98f9e60 100644 --- a/internal/cmdupdate/cmdupdate_test.go +++ b/internal/cmdupdate/cmdupdate_test.go @@ -15,17 +15,22 @@ package cmdupdate_test import ( + "bytes" "io/ioutil" "os" "path/filepath" + "regexp" "runtime" + "strings" "testing" + "text/template" "github.com/GoogleContainerTools/kpt/internal/cmdget" "github.com/GoogleContainerTools/kpt/internal/cmdupdate" "github.com/GoogleContainerTools/kpt/internal/gitutil" "github.com/GoogleContainerTools/kpt/internal/printer/fake" "github.com/GoogleContainerTools/kpt/internal/testutil" + "github.com/GoogleContainerTools/kpt/internal/testutil/pkgbuilder" kptfilev1alpha2 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1alpha2" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -330,3 +335,363 @@ func TestCmd_path(t *testing.T) { }) } } + +func TestCmd_output(t *testing.T) { + testCases := map[string]struct { + reposChanges map[string][]testutil.Content + updatedLocal testutil.Content + expectedLocal *pkgbuilder.RootPkg + expectedOutput string + }{ + "basic package": { + reposChanges: map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "master", + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.SecretResource), + }, + }, + }, + expectedLocal: pkgbuilder.NewRootPkg(). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef(testutil.Upstream, "/", "master", "resource-merge"). + WithUpstreamLockRef(testutil.Upstream, "/", "master", 1), + ). + WithResource(pkgbuilder.SecretResource), + expectedOutput: ` +Package "{{ .PKG_NAME }}": +Fetching upstream from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Fetching origin from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Updating package "{{ .PKG_NAME }}" with strategy "resource-merge". + +Updated 1 package(s). +`, + }, + "nested packages": { + reposChanges: map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource). + WithSubPackages( + pkgbuilder.NewSubPkg("subpkg"). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef("foo", "/", "master", "fast-forward"). + WithUpstreamLockRef("foo", "/", "master", 0), + ). + WithResource(pkgbuilder.DeploymentResource), + ), + Branch: "master", + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.SecretResource). + WithSubPackages( + pkgbuilder.NewSubPkg("subpkg"). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef("foo", "/", "master", "fast-forward"). + WithUpstreamLockRef("foo", "/", "master", 0), + ). + WithResource(pkgbuilder.DeploymentResource), + ), + }, + }, + "foo": { + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "master", + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.ConfigMapResource), + }, + }, + }, + expectedLocal: pkgbuilder.NewRootPkg(). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef(testutil.Upstream, "/", "master", "resource-merge"). + WithUpstreamLockRef(testutil.Upstream, "/", "master", 1), + ). + WithResource(pkgbuilder.SecretResource). + WithSubPackages( + pkgbuilder.NewSubPkg("subpkg"). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef("foo", "/", "master", "fast-forward"). + WithUpstreamLockRef("foo", "/", "master", 1), + ). + WithResource(pkgbuilder.ConfigMapResource), + ), + expectedOutput: ` +Package "{{ .PKG_NAME }}": +Fetching upstream from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Fetching origin from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Updating package "{{ .PKG_NAME }}" with strategy "resource-merge". +Updating package "subpkg" with strategy "fast-forward". + +Package "{{ .PKG_NAME }}/subpkg": +Fetching upstream from {{ (index .REPOS "foo").RepoDirectory }}@master + +Fetching origin from {{ (index .REPOS "foo").RepoDirectory }}@master + +Updating package "subpkg" with strategy "fast-forward". + +Updated 2 package(s). +`, + }, + "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"). + WithUpstreamLockRef("foo", "/", "master", 0), + ). + WithResource(pkgbuilder.DeploymentResource), + pkgbuilder.NewSubPkg("subpkg2"). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef("foo", "/", "master", "resource-merge"). + WithUpstreamLockRef("foo", "/", "master", 0), + ). + WithResource(pkgbuilder.DeploymentResource), + ), + Branch: "master", + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.SecretResource), + }, + }, + "foo": { + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "master", + }, + + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource). + WithResource(pkgbuilder.ConfigMapResource), + }, + }, + }, + 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", 1), + ). + WithResource(pkgbuilder.ConfigMapResource). + WithResource(pkgbuilder.DeploymentResource, pkgbuilder.SetFieldPath("5", "spec", "replicas")), + ), + expectedOutput: ` +Package "{{ .PKG_NAME }}": +Fetching upstream from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Fetching origin from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Updating package "{{ .PKG_NAME }}" with strategy "resource-merge". +Deleting package "subpkg2" from local since it is removed in upstream. +Package "subpkg1" deleted from upstream, but keeping local since it has changes. + +Package "{{ .PKG_NAME }}/subpkg1": +Fetching upstream from {{ (index .REPOS "foo").RepoDirectory }}@master + +Fetching origin from {{ (index .REPOS "foo").RepoDirectory }}@master + +Updating package "subpkg1" with strategy "resource-merge". + +Updated 2 package(s). +`, + }, + "Adding package in upstream": { + reposChanges: map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "master", + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.SecretResource). + WithSubPackages( + pkgbuilder.NewSubPkg("subpkg"). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef("foo", "/", "v1", "force-delete-replace"), + ), + ), + }, + }, + "foo": { + { + Pkg: pkgbuilder.NewRootPkg(). + WithKptfile(). + WithResource(pkgbuilder.DeploymentResource), + Branch: "master", + Tag: "v1", + }, + }, + }, + expectedLocal: pkgbuilder.NewRootPkg(). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef(testutil.Upstream, "/", "master", "resource-merge"). + WithUpstreamLockRef(testutil.Upstream, "/", "master", 1), + ). + WithResource(pkgbuilder.SecretResource). + WithSubPackages( + pkgbuilder.NewSubPkg("subpkg"). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef("foo", "/", "v1", "force-delete-replace"). + WithUpstreamLockRef("foo", "/", "v1", 0), + ). + WithResource(pkgbuilder.DeploymentResource), + ), + expectedOutput: ` +Package "{{ .PKG_NAME }}": +Fetching upstream from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Fetching origin from {{ (index .REPOS "upstream").RepoDirectory }}@master + +Updating package "{{ .PKG_NAME }}" with strategy "resource-merge". +Adding package "subpkg" from upstream. + +Package "{{ .PKG_NAME }}/subpkg": +Fetching upstream from {{ (index .REPOS "foo").RepoDirectory }}@v1 + +Updating package "subpkg" with strategy "force-delete-replace". + +Updated 2 package(s). +`, + }, + } + + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + g := &testutil.TestSetupManager{ + T: t, + ReposChanges: tc.reposChanges, + } + defer g.Clean() + if tc.updatedLocal.Pkg != nil { + g.LocalChanges = []testutil.Content{ + tc.updatedLocal, + } + } + if !g.Init() { + return + } + + clean := testutil.Chdir(t, g.LocalWorkspace.FullPackagePath()) + defer clean() + + var outBuf bytes.Buffer + var errBuf bytes.Buffer + + ctx := fake.CtxWithPrinter(&outBuf, &errBuf) + r := cmdupdate.NewRunner(ctx, "kpt") + r.Command.SetArgs([]string{}) + err := r.Command.Execute() + if !assert.NoError(t, err) { + t.FailNow() + } + + assert.Empty(t, outBuf.String()) + + tmpl := template.Must(template.New("test").Parse(tc.expectedOutput)) + var expected bytes.Buffer + err = tmpl.Execute(&expected, map[string]interface{}{ + "PKG_PATH": g.LocalWorkspace.FullPackagePath(), + "PKG_NAME": g.LocalWorkspace.PackageDir, + "REPOS": g.Repos, + }) + if !assert.NoError(t, err) { + t.FailNow() + } + actual := scrubGitOutput(errBuf.String()) + + assert.Equal(t, strings.TrimSpace(expected.String()), strings.TrimSpace(actual)) + + expectedPath := tc.expectedLocal.ExpandPkgWithName(t, g.LocalWorkspace.PackageDir, testutil.ToReposInfo(g.Repos)) + testutil.KptfileAwarePkgEqual(t, expectedPath, g.LocalWorkspace.FullPackagePath(), true) + }) + } +} + +const ( + gitOutputPattern = `From \/.*(\r\n|\r|\n)( * .*(\r\n|\r|\n))+` +) + +func scrubGitOutput(output string) string { + re := regexp.MustCompile(gitOutputPattern) + return re.ReplaceAllString(output, "\n") +} diff --git a/internal/printer/fake/fake.go b/internal/printer/fake/fake.go index 513c4af891..e790ae21fa 100644 --- a/internal/printer/fake/fake.go +++ b/internal/printer/fake/fake.go @@ -47,8 +47,5 @@ func CtxWithDefaultPrinter() context.Context { // CtxWithPrinter returns a new context with Printer added. func CtxWithPrinter(outStream, errStream io.Writer) context.Context { ctx := context.Background() - return printer.WithContext(ctx, &Printer{ - outStream: outStream, - errStream: errStream, - }) + return printer.WithContext(ctx, printer.New(outStream, errStream)) } diff --git a/internal/testutil/pkgbuilder/builder.go b/internal/testutil/pkgbuilder/builder.go index fcd8f7e6fb..7939d7e3a2 100644 --- a/internal/testutil/pkgbuilder/builder.go +++ b/internal/testutil/pkgbuilder/builder.go @@ -511,28 +511,27 @@ func buildPkg(pkgPath string, pkg *pkg, pkgName string, reposInfo ReposInfo) err } // TODO: Consider using the Kptfile struct for this instead of a template. -var kptfileTemplate = ` -apiVersion: kpt.dev/v1alpha2 +var kptfileTemplate = `apiVersion: kpt.dev/v1alpha2 kind: Kptfile metadata: name: {{.PkgName}} {{- if .Pkg.Kptfile.Upstream }} upstream: type: git - updateStrategy: {{.Pkg.Kptfile.Upstream.Strategy}} git: + repo: {{.Pkg.Kptfile.Upstream.Repo}} directory: {{.Pkg.Kptfile.Upstream.Dir}} ref: {{.Pkg.Kptfile.Upstream.Ref}} - repo: {{.Pkg.Kptfile.Upstream.Repo}} + updateStrategy: {{.Pkg.Kptfile.Upstream.Strategy}} {{- end }} {{- if .Pkg.Kptfile.UpstreamLock }} upstreamLock: type: git git: - commit: {{.Pkg.Kptfile.UpstreamLock.Commit}} + repo: {{.Pkg.Kptfile.UpstreamLock.Repo}} directory: {{.Pkg.Kptfile.UpstreamLock.Dir}} ref: {{.Pkg.Kptfile.UpstreamLock.Ref}} - repo: {{.Pkg.Kptfile.UpstreamLock.Repo}} + commit: {{.Pkg.Kptfile.UpstreamLock.Commit}} {{- end }} {{- if .Pkg.Kptfile.Pipeline }} pipeline: diff --git a/internal/util/pkgutil/pkgutil.go b/internal/util/pkgutil/pkgutil.go index 0689ce931b..84e85a3672 100644 --- a/internal/util/pkgutil/pkgutil.go +++ b/internal/util/pkgutil/pkgutil.go @@ -239,9 +239,16 @@ func RootPkgFirstSorter(paths []string) func(i, j int) bool { if jPath == "." { return false } + // First sort based on the number of segments. + // TODO: Verify whether this works on Windows. It looks like it + // probably wont. iSegmentCount := len(strings.Split(iPath, "/")) jSegmentCount := len(strings.Split(jPath, "/")) - return iSegmentCount < jSegmentCount + if jSegmentCount != iSegmentCount { + return iSegmentCount < jSegmentCount + } + // If two paths are at the same depth, just sort lexicographically. + return iPath < jPath } } From 7767a85dbcc09becaed93bf00b4a9199526da61d Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Sat, 12 Jun 2021 14:58:30 -0700 Subject: [PATCH 2/2] Fix incorrect handling when unfetched packages are deleted from upstream --- internal/util/fetch/fetch.go | 55 +++++++++++++----- internal/util/fetch/fetch_test.go | 86 ++++++++++++++++++++++++++++- internal/util/update/update.go | 40 +++++++++++++- internal/util/update/update_test.go | 82 +++++++++++++++++++++++++++ 4 files changed, 246 insertions(+), 17 deletions(-) diff --git a/internal/util/fetch/fetch.go b/internal/util/fetch/fetch.go index d084643725..b56405c3a7 100644 --- a/internal/util/fetch/fetch.go +++ b/internal/util/fetch/fetch.go @@ -39,7 +39,17 @@ 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. + // After the command succeeds, this field will contain the commit sha + // of the upstream package that was fetched. + Commit string } // Run runs the Command. @@ -59,6 +69,7 @@ func (c Command) Run(ctx context.Context) error { OrgRepo: g.Repo, Path: g.Directory, Ref: g.Ref, + Commit: c.Commit, } err = cloneAndCopy(ctx, repoSpec, c.Pkg.UniquePath.String()) if err != nil { @@ -138,21 +149,20 @@ func ClonerUsingGitExec(ctx context.Context, repoSpec *git.RepoSpec) error { return errors.E(op, errors.Git, errors.Repo(repoSpec.CloneSpec()), err) } - // 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(repoSpec.Path, "/") - for len(ps) != 0 { - p := path.Join(ps...) - packageRef := path.Join(strings.TrimLeft(p, "/"), repoSpec.Ref) - if _, found := upstreamRepo.ResolveTag(packageRef); found { - repoSpec.Ref = packageRef - break - } - ps = ps[:len(ps)-1] + var ref string + // If a commit sha is provided in the repoSpec, we use it instead of the + // Ref value. + if repoSpec.Commit != "" { + ref = repoSpec.Commit + } else { + // If we are using the ref value, check for the most package specific + // tag that matches the provided ref. + ref = checkPackageTags(repoSpec.Path, repoSpec.Ref, upstreamRepo) + repoSpec.Ref = ref } // Pull the required ref into the repo git cache. - dir, err := upstreamRepo.GetRepo(ctx, []string{repoSpec.Ref}) + dir, err := upstreamRepo.GetRepo(ctx, []string{ref}) if err != nil { return errors.E(op, errors.Git, errors.Repo(repoSpec.CloneSpec()), err) } @@ -164,9 +174,9 @@ func ClonerUsingGitExec(ctx context.Context, repoSpec *git.RepoSpec) error { // 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, found := upstreamRepo.ResolveRef(repoSpec.Ref) + commit, found := upstreamRepo.ResolveRef(ref) if !found { - commit = repoSpec.Ref + commit = ref } // Reset the local repo to the commit we need. Doing a hard reset instead of @@ -223,3 +233,20 @@ func ClonerUsingGitExec(ctx context.Context, repoSpec *git.RepoSpec) error { } return nil } + +// checkPackageTags looks up most specific tag for the package at the provided +// pkgPath. +func checkPackageTags(pkgPath, ref string, upstreamRepo *gitutil.GitUpstreamRepo) string { + // 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(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 +} diff --git a/internal/util/fetch/fetch_test.go b/internal/util/fetch/fetch_test.go index 96f6448853..0f6db89312 100644 --- a/internal/util/fetch/fetch_test.go +++ b/internal/util/fetch/fetch_test.go @@ -50,13 +50,18 @@ func setupWorkspace(t *testing.T) (*testutil.TestGitRepo, *testutil.TestWorkspac } func createKptfile(workspace *testutil.TestWorkspace, git *kptfilev1alpha2.Git, strategy kptfilev1alpha2.UpdateStrategyType) error { - kf := kptfileutil.DefaultKptfile(workspace.PackageDir) + kf := newKptfile(workspace, git, strategy) + return kptfileutil.WriteFile(workspace.FullPackagePath(), kf) +} + +func newKptfile(w *testutil.TestWorkspace, git *kptfilev1alpha2.Git, strategy kptfilev1alpha2.UpdateStrategyType) *kptfilev1alpha2.KptFile { + kf := kptfileutil.DefaultKptfile(w.PackageDir) kf.Upstream = &kptfilev1alpha2.Upstream{ Type: kptfilev1alpha2.GitOrigin, Git: git, UpdateStrategy: strategy, } - return kptfileutil.WriteFile(workspace.FullPackagePath(), kf) + return kf } func setKptfileName(workspace *testutil.TestWorkspace, name string) error { @@ -696,3 +701,80 @@ func TestCommand_Run_failInvalidTag(t *testing.T) { t.FailNow() } } + +func TestCommand_Run_specificCommit(t *testing.T) { + testCases := map[string]struct { + setCommit bool + expected *pkgbuilder.RootPkg + }{ + "ref is branch without commit specified": { + setCommit: false, + expected: pkgbuilder.NewRootPkg(). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef(testutil.Upstream, "/", "foo", "resource-merge"). + WithUpstreamLockRef(testutil.Upstream, "/", "foo", 1), + ). + WithResource(pkgbuilder.DeploymentResource), + }, + "ref is branch with commit specified": { + setCommit: true, + expected: pkgbuilder.NewRootPkg(). + WithKptfile( + pkgbuilder.NewKptfile(). + WithUpstreamRef(testutil.Upstream, "/", "foo", "resource-merge"). + WithUpstreamLockRef(testutil.Upstream, "/", "foo", 0), + ). + WithResource(pkgbuilder.SecretResource), + }, + } + + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + repoContent := map[string][]testutil.Content{ + testutil.Upstream: { + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.SecretResource), + Branch: "foo", + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.DeploymentResource), + }, + }, + } + repos, w, clean := testutil.SetupReposAndWorkspace(t, repoContent) + defer clean() + if err := testutil.UpdateRepos(t, repos, repoContent); err != nil { + t.FailNow() + } + + g := repos[testutil.Upstream] + w.PackageDir = g.RepoName + + kf := newKptfile(w, &kptfilev1alpha2.Git{ + Repo: g.RepoDirectory, + Directory: "/", + Ref: "foo", + }, kptfilev1alpha2.ResourceMerge) + testutil.AddKptfileToWorkspace(t, w, kf) + + actualPkg := pkgtesting.CreatePkgOrFail(t, w.FullPackagePath()) + var commit string + if tc.setCommit { + commit = g.Commits[0] + } + err := Command{ + Pkg: actualPkg, + Commit: commit, + }.Run(fake.CtxWithDefaultPrinter()) + if !assert.NoError(t, err) { + t.FailNow() + } + + expectedPath := tc.expected.ExpandPkgWithName(t, w.PackageDir, testutil.ToReposInfo(repos)) + testutil.KptfileAwarePkgEqual(t, expectedPath, w.FullPackagePath(), false) + }) + } +} diff --git a/internal/util/update/update.go b/internal/util/update/update.go index 2836002478..12db07c6ae 100644 --- a/internal/util/update/update.go +++ b/internal/util/update/update.go @@ -374,7 +374,45 @@ func (u Command) updatePackage(ctx context.Context, subPkgPath, localPath, updat // Package deleted from upstream case originExists && localExists && !updatedExists: - // Check the diff. If there are local changes, we keep the subpackage. + // 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, types.UniquePath(localPath), err) + } + // If the package in origin is in the unfetched state, we need to fetch + // it before we can diff with local. + 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 := pkg.ReadKptfile(localPath) + if err != nil { + return errors.E(op, types.UniquePath(localPath), err) + } + localOriginSha := localKf.UpstreamLock.Git.Commit + p, err := pkg.New(originPath) + if err != nil { + return errors.E(op, types.UniquePath(localPath), err) + } + // 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 { + return errors.E(op, types.UniquePath(localPath), err) + } + // Add the merge comments to make sure we don't get unnecessary + // diffs. + if err := addmergecomment.Process(originPath); err != nil { + return errors.E(op, types.UniquePath(localPath), err) + } + } + // Perform a diff to check if local has changes compared to origin. diff, err := copyutil.Diff(originPath, localPath) if err != nil { return errors.E(op, types.UniquePath(localPath), err) diff --git a/internal/util/update/update_test.go b/internal/util/update/update_test.go index a56e20ebd9..fa13145eb5 100644 --- a/internal/util/update/update_test.go +++ b/internal/util/update/update_test.go @@ -3291,6 +3291,88 @@ func TestRun_remote_subpackages(t *testing.T) { WithResource(pkgbuilder.ConfigMapResource), ), }, + "unfetched subpackage is deleted in upstream": { + 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, + }, + { + Pkg: pkgbuilder.NewRootPkg(). + WithResource(pkgbuilder.ConfigMapResource). + WithResource(pkgbuilder.DeploymentResource), + }, + }, + }, + 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, 1), + ). + WithResource(pkgbuilder.ConfigMapResource). + WithResource(pkgbuilder.DeploymentResource, pkgbuilder.SetFieldPath("5", "spec", "replicas")), + ), + }, } for tn, tc := range testCases {