From 1046ea26a1b7cda65e2fb4d5df729e85fa9a5bc0 Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Fri, 10 Mar 2023 23:02:20 +0000 Subject: [PATCH] [WIP] Fix the edit/copy of packagerevisions (#3863) --- commands/alpha/rpkg/copy/command.go | 20 +- .../porch/v1alpha1/types_packagerevisions.go | 19 + porch/pkg/engine/edit.go | 26 +- porch/pkg/engine/edit_test.go | 20 +- porch/pkg/engine/engine.go | 144 +++++- porch/pkg/engine/engine_test.go | 448 ++++++++++++++++++ porch/pkg/engine/fake/repository.go | 19 +- porch/test/e2e/e2e_test.go | 112 +++++ 8 files changed, 770 insertions(+), 38 deletions(-) diff --git a/commands/alpha/rpkg/copy/command.go b/commands/alpha/rpkg/copy/command.go index a47b87b27c..c301f25e27 100644 --- a/commands/alpha/rpkg/copy/command.go +++ b/commands/alpha/rpkg/copy/command.go @@ -53,6 +53,7 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner Hidden: porch.HidePorchCommands, } r.Command.Flags().StringVar(&r.workspace, "workspace", "", "Workspace name of the copy of the package.") + r.Command.Flags().BoolVar(&r.replayStrategy, "replay-strategy", true, "Use replay strategy for creating new package revision.") return r } @@ -64,7 +65,8 @@ type runner struct { copy porchapi.PackageEditTaskSpec - workspace string // Target package revision workspaceName + workspace string // Target package revision workspaceName + replayStrategy bool } func (r *runner) preRunE(cmd *cobra.Command, args []string) error { @@ -132,6 +134,20 @@ func (r *runner) getPackageRevisionSpec() (*porchapi.PackageRevisionSpec, error) WorkspaceName: porchapi.WorkspaceName(r.workspace), RepositoryName: packageRevision.Spec.RepositoryName, } - spec.Tasks = packageRevision.Spec.Tasks + + if len(packageRevision.Spec.Tasks) == 0 || !r.replayStrategy { + spec.Tasks = []porchapi.Task{ + { + Type: porchapi.TaskTypeEdit, + Edit: &porchapi.PackageEditTaskSpec{ + Source: &porchapi.PackageRevisionRef{ + Name: packageRevision.Name, + }, + }, + }, + } + } else { + spec.Tasks = packageRevision.Spec.Tasks + } return spec, nil } diff --git a/porch/api/porch/v1alpha1/types_packagerevisions.go b/porch/api/porch/v1alpha1/types_packagerevisions.go index cb0f55f58f..6c8dbf71a0 100644 --- a/porch/api/porch/v1alpha1/types_packagerevisions.go +++ b/porch/api/porch/v1alpha1/types_packagerevisions.go @@ -79,6 +79,25 @@ type PackageRevisionSpec struct { Lifecycle PackageRevisionLifecycle `json:"lifecycle,omitempty"` + // The task slice holds zero or more tasks that describe the operations + // performed on the packagerevision. The are essentially a replayable history + // of the packagerevision, + // + // Packagerevisions that were not created in Porch may have an + // empty task list. + // + // Packagerevisions created and managed through Porch will always + // have either an Init, Edit, or a Clone task as the first entry in their + // task list. This represent packagerevisions created from scratch, based + // a copy of a different revision in the same package, or a packagerevision + // cloned from another package. + // Each change to the packagerevision will result in a correspondig + // task being added to the list of tasks. It will describe the operation + // performed and will have a corresponding entry (commit or layer) in git + // or oci. + // The task slice describes the history of the packagerevision, so it + // is an append only list (We might introduce some kind of compaction in the + // future to keep the number of tasks at a reasonable number). Tasks []Task `json:"tasks,omitempty"` ReadinessGates []ReadinessGate `json:"readinessGates,omitempty"` diff --git a/porch/pkg/engine/edit.go b/porch/pkg/engine/edit.go index 9213b038e0..152d6db3ba 100644 --- a/porch/pkg/engine/edit.go +++ b/porch/pkg/engine/edit.go @@ -26,6 +26,8 @@ import ( type editPackageMutation struct { task *api.Task namespace string + repositoryName string + packageName string repoOpener RepositoryOpener referenceResolver ReferenceResolver } @@ -38,15 +40,31 @@ func (m *editPackageMutation) Apply(ctx context.Context, resources repository.Pa sourceRef := m.task.Edit.Source - sourceResources, err := (&PackageFetcher{ + revision, err := (&PackageFetcher{ repoOpener: m.repoOpener, referenceResolver: m.referenceResolver, - }).FetchResources(ctx, sourceRef, m.namespace) + }).FetchRevision(ctx, sourceRef, m.namespace) if err != nil { - return repository.PackageResources{}, nil, fmt.Errorf("failed to fetch resources for package %q: %w", sourceRef.Name, err) + return repository.PackageResources{}, nil, fmt.Errorf("failed to fetch package %q: %w", sourceRef.Name, err) + } + + // We only allow edit to create new revision from the same package. + if revision.Key().Package != m.packageName || + revision.Key().Repository != m.repositoryName { + return repository.PackageResources{}, nil, fmt.Errorf("source revision must be from same package %s/%s", m.repositoryName, m.packageName) + } + + // We only allow edit to create new revisions from published packages. + if !api.LifecycleIsPublished(revision.Lifecycle()) { + return repository.PackageResources{}, nil, fmt.Errorf("source revision must be published") + } + + sourceResources, err := revision.GetResources(ctx) + if err != nil { + return repository.PackageResources{}, nil, fmt.Errorf("cannot read contents of package %q: %w", sourceRef.Name, err) } return repository.PackageResources{ Contents: sourceResources.Spec.Resources, - }, &api.TaskResult{Task: &api.Task{}}, nil + }, &api.TaskResult{Task: m.task}, nil } diff --git a/porch/pkg/engine/edit_test.go b/porch/pkg/engine/edit_test.go index b02812dba1..011db9b06c 100644 --- a/porch/pkg/engine/edit_test.go +++ b/porch/pkg/engine/edit_test.go @@ -28,14 +28,23 @@ import ( ) func TestEdit(t *testing.T) { - packageName := "foo-1234567890" + pkg := "pkg" + packageName := "repo-1234567890" + repositoryName := "repo" + revision := "v1" packageRevision := &fake.PackageRevision{ Name: packageName, + PackageRevisionKey: repository.PackageRevisionKey{ + Package: pkg, + Repository: repositoryName, + Revision: revision, + }, + PackageLifecycle: v1alpha1.PackageRevisionLifecyclePublished, Resources: &v1alpha1.PackageRevisionResources{ Spec: v1alpha1.PackageRevisionResourcesSpec{ - PackageName: packageName, - Revision: "v1", - RepositoryName: "foo", + PackageName: pkg, + Revision: revision, + RepositoryName: repositoryName, Resources: map[string]string{ kptfile.KptFileName: strings.TrimSpace(` apiVersion: kpt.dev/v1 @@ -69,7 +78,10 @@ info: }, }, }, + namespace: "test-namespace", + packageName: pkg, + repositoryName: repositoryName, referenceResolver: &fakeReferenceResolver{}, repoOpener: repoOpener, } diff --git a/porch/pkg/engine/engine.go b/porch/pkg/engine/engine.go index 951d00b380..28996a1876 100644 --- a/porch/pkg/engine/engine.go +++ b/porch/pkg/engine/engine.go @@ -295,17 +295,17 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * return nil, fmt.Errorf("failed to create packagerevision: %w", err) } - // The workspaceName must be unique, because it used to generate the package revision's metadata.name. - revs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{Package: obj.Spec.PackageName, WorkspaceName: obj.Spec.WorkspaceName}) + revs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Package: obj.Spec.PackageName}) if err != nil { - return nil, fmt.Errorf("error searching through existing package revisions: %w", err) + return nil, fmt.Errorf("error listing package revisions: %w", err) } - if len(revs) != 0 { - return nil, fmt.Errorf("package revision workspaceNames must be unique; package revision with name %s in repo %s with "+ - "workspaceName %s already exists", obj.Spec.PackageName, obj.Spec.RepositoryName, obj.Spec.WorkspaceName) + + if err := ensureUniqueWorkspaceName(obj, revs); err != nil { + return nil, err } - sameOrigin, err := cad.ensureSameOrigin(ctx, obj, repo) + sameOrigin, err := ensureSameOrigin(ctx, repo, obj, revs) if err != nil { return nil, fmt.Errorf("error ensuring same origin: %w", err) } @@ -351,29 +351,44 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj * }, nil } -func (cad *cadEngine) ensureSameOrigin(ctx context.Context, obj *api.PackageRevision, r repository.Repository) (bool, error) { - revs, err := r.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ - Package: obj.Spec.PackageName}) - if err != nil { - return false, fmt.Errorf("error listing package revisions: %w", err) +// The workspaceName must be unique, because it used to generate the package revision's metadata.name. +func ensureUniqueWorkspaceName(obj *api.PackageRevision, existingRevs []repository.PackageRevision) error { + for _, r := range existingRevs { + k := r.Key() + if k.WorkspaceName == obj.Spec.WorkspaceName { + return fmt.Errorf("package revision workspaceNames must be unique; package revision with name %s in repo %s with "+ + "workspaceName %s already exists", obj.Spec.PackageName, obj.Spec.RepositoryName, obj.Spec.WorkspaceName) + } } - if len(revs) == 0 { + return nil +} + +func ensureSameOrigin(ctx context.Context, repo repository.Repository, obj *api.PackageRevision, existingRevs []repository.PackageRevision) (bool, error) { + if len(existingRevs) == 0 { // no prior package revisions, no need to check anything else return true, nil } tasks := obj.Spec.Tasks - if len(tasks) == 0 || (tasks[0].Type != api.TaskTypeInit && tasks[0].Type != api.TaskTypeClone) { + if len(tasks) == 0 || !taskTypeOneOf(tasks[0].Type, api.TaskTypeInit, api.TaskTypeClone, api.TaskTypeEdit) { // If there are no tasks, or the first task is not init or clone, then this revision was not // created from another package revision. That means we expect it to be the first revision // for this package. return false, nil - } firstObjTask := tasks[0].DeepCopy() - // iterate over existing package revisions, and look for one with a matching init or clone task - for _, rev := range revs { + + // Edit tasks is a copy of a different revision in the same package, so therefore must + // always be allowed. + if firstObjTask.Type == api.TaskTypeEdit { + return true, nil + } + + // iterate over existing package revisions, and look for one with a matching init or clone task. + // We consider a packagerevision to have the same upstream if we find at least one existing + // packagerevision that matches. + for _, rev := range existingRevs { p, err := rev.GetPackageRevision(ctx) if err != nil { return false, err @@ -388,31 +403,106 @@ func (cad *cadEngine) ensureSameOrigin(ctx context.Context, obj *api.PackageRevi // not a match continue } + if firstObjTask.Type == api.TaskTypeClone { // the user is allowed to update the git upstream ref, so we exclude that from the equality check. // We make the git upstream refs equal before calling reflect.DeepEqual - if firstRevTask.Clone != nil && firstObjTask.Clone != nil { - if firstRevTask.Clone.Upstream.Git != nil && firstObjTask.Clone.Upstream.Git != nil { - firstRevTask.Clone.Upstream.Git.Ref = firstObjTask.Clone.Upstream.Git.Ref + if firstRevTask.Clone == nil || firstObjTask.Clone == nil { + continue + } + + switch { + // If both the new and existing packagerevision has a git upstream, we consider it + // from the same upstream if the repo and directory are the same. + case firstRevTask.Clone.Upstream.Git != nil && firstObjTask.Clone.Upstream.Git != nil: + firstRevTask.Clone.Upstream.Git.Ref = firstObjTask.Clone.Upstream.Git.Ref + if reflect.DeepEqual(firstRevTask, firstObjTask) { + return true, nil } - if firstRevTask.Clone.Upstream.UpstreamRef != nil && firstObjTask.Clone.Upstream.UpstreamRef != nil { - firstRevTask.Clone.Upstream.UpstreamRef = firstObjTask.Clone.Upstream.UpstreamRef + // If both the new and existing packagerevision has an oci upstream, we consider it + // from the same upstream if the image path is the same. We do not care about any tag + // or digest. + case firstRevTask.Clone.Upstream.Oci != nil && firstObjTask.Clone.Upstream.Oci != nil: + objOciImage := getBaseImage(firstObjTask.Clone.Upstream.Oci.Image) + revOciImage := getBaseImage(firstRevTask.Clone.Upstream.Oci.Image) + if objOciImage == revOciImage { + firstRevTask.Clone.Upstream.Oci.Image = firstObjTask.Clone.Upstream.Oci.Image + } + if reflect.DeepEqual(firstRevTask, firstObjTask) { + return true, nil + } + // If both the new and existing packagerevision has a porch upstream, we dereference the + // upstreamRef and make sure that both reference revisions of the same package. + case firstRevTask.Clone.Upstream.UpstreamRef != nil && firstObjTask.Clone.Upstream.UpstreamRef != nil: + revRepoPkgRev, found, err := getPackageRevision(ctx, repo, firstObjTask.Clone.Upstream.UpstreamRef.Name) + if err != nil { + return false, err + } + if !found { + continue + } + + objRepoPkgRev, found, err := getPackageRevision(ctx, repo, firstRevTask.Clone.Upstream.UpstreamRef.Name) + if err != nil { + return false, err } + if !found { + continue + } + if revRepoPkgRev.Key().Repository == objRepoPkgRev.Key().Repository && + revRepoPkgRev.Key().Package == objRepoPkgRev.Key().Package { + return true, nil + } + + } + } else { + if reflect.DeepEqual(firstRevTask, firstObjTask) { + return true, nil } - } - if reflect.DeepEqual(firstRevTask, firstObjTask) { - return true, nil } } return false, nil } +func getPackageRevision(ctx context.Context, repo repository.Repository, name string) (repository.PackageRevision, bool, error) { + repoPkgRevs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + KubeObjectName: name, + }) + if err != nil { + return nil, false, err + } + if len(repoPkgRevs) == 0 { + return nil, false, nil + } + return repoPkgRevs[0], true, nil +} + +// TODO: See if we can use a library here for parsing OCI image urls +func getBaseImage(image string) string { + if s := strings.Split(image, "@sha256:"); len(s) > 1 { + return s[0] + } + if s := strings.Split(image, ":"); len(s) > 1 { + return s[0] + } + return image +} + +func taskTypeOneOf(taskType api.TaskType, oneOf ...api.TaskType) bool { + for _, tt := range oneOf { + if taskType == tt { + return true + } + } + return false +} + func (cad *cadEngine) applyTasks(ctx context.Context, draft repository.PackageDraft, repositoryObj *configapi.Repository, obj *api.PackageRevision, packageConfig *builtins.PackageConfig) error { var mutations []mutation // Unless first task is Init or Clone, insert Init to create an empty package. tasks := obj.Spec.Tasks - if len(tasks) == 0 || (tasks[0].Type != api.TaskTypeInit && tasks[0].Type != api.TaskTypeClone) { + if len(tasks) == 0 || !taskTypeOneOf(tasks[0].Type, api.TaskTypeInit, api.TaskTypeClone, api.TaskTypeEdit) { mutations = append(mutations, &initPackageMutation{ name: obj.Spec.PackageName, task: &api.Task{ @@ -500,6 +590,8 @@ func (cad *cadEngine) mapTaskToMutation(ctx context.Context, obj *api.PackageRev return &editPackageMutation{ task: task, namespace: obj.Namespace, + packageName: obj.Spec.PackageName, + repositoryName: obj.Spec.RepositoryName, repoOpener: cad, referenceResolver: cad.referenceResolver, }, nil diff --git a/porch/pkg/engine/engine_test.go b/porch/pkg/engine/engine_test.go index d40cc15332..fd622de71e 100644 --- a/porch/pkg/engine/engine_test.go +++ b/porch/pkg/engine/engine_test.go @@ -315,3 +315,451 @@ func TestSomething(t *testing.T) { }) } } + +func TestEnsureSameOrigin(t *testing.T) { + testCases := map[string]struct { + obj *api.PackageRevision + existingRevs []repository.PackageRevision + repoRevs []repository.PackageRevision + hasSameOrigin bool + expectedErr error + }{ + "init task, no existing packagerevisions": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeInit, + Init: &api.PackageInitTaskSpec{}, + }, + }, + }, + }, + existingRevs: []repository.PackageRevision{}, + hasSameOrigin: true, + expectedErr: nil, + }, + "init task, existing packagerevision with first init task": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeInit, + Init: &api.PackageInitTaskSpec{}, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeInit, + Init: &api.PackageInitTaskSpec{}, + }, + ), + hasSameOrigin: true, + expectedErr: nil, + }, + "init task, existing packagerevision with another first task": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeInit, + Init: &api.PackageInitTaskSpec{}, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task, no existing packagerevisions": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{}, + }, + }, + }, + }, + existingRevs: []repository.PackageRevision{}, + hasSameOrigin: true, + expectedErr: nil, + }, + "clone task, existing packagerevision with different first task": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{}, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeInit, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task git, existing packagerevision with different source type": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeGit, + Git: &api.GitPackage{ + Repo: "https://github.com/GoogleContainerTools/kpt", + Ref: "main", + Directory: "/", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeOCI, + }, + }, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task git, existing packagerevision with different repo": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeGit, + Git: &api.GitPackage{ + Repo: "https://github.com/GoogleContainerTools/kpt", + Ref: "main", + Directory: "/", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeGit, + Git: &api.GitPackage{ + Repo: "https://github.com/GoogleContainerTools/somethingelse", + Ref: "main", + Directory: "/", + }, + }, + }, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task git, existing packagerevision with same repo": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeGit, + Git: &api.GitPackage{ + Repo: "https://github.com/GoogleContainerTools/kpt", + Ref: "main", + Directory: "/", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeGit, + Git: &api.GitPackage{ + Repo: "https://github.com/GoogleContainerTools/kpt", + Ref: "main", + Directory: "/", + }, + }, + }, + }, + ), + hasSameOrigin: true, + expectedErr: nil, + }, + "clone task oci, existing packagerevision with different image": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeOCI, + Oci: &api.OciPackage{ + Image: "gcr.io/kpt-dev/image", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeOCI, + Oci: &api.OciPackage{ + Image: "gcr.io/kpt-dev/otherimage", + }, + }, + }, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task oci, existing packagerevision with same image": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeOCI, + Oci: &api.OciPackage{ + Image: "gcr.io/kpt-dev/image:foo", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeOCI, + Oci: &api.OciPackage{ + Image: "gcr.io/kpt-dev/image:bar", + }, + }, + }, + }, + ), + hasSameOrigin: true, + expectedErr: nil, + }, + "clone task porch, existing packagerevision with different source type": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + UpstreamRef: &api.PackageRevisionRef{ + Name: "foo-12345", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + Type: api.RepositoryTypeOCI, + Oci: &api.OciPackage{ + Image: "gcr.io/kpt-dev/image:bar", + }, + }, + }, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task porch, existing packagerevision with upstream ref to different package": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + UpstreamRef: &api.PackageRevisionRef{ + Name: "foo-12345", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + UpstreamRef: &api.PackageRevisionRef{ + Name: "bar-12345", + }, + }, + }, + }, + ), + hasSameOrigin: false, + expectedErr: nil, + }, + "clone task porch, existing packagerevision with upstream ref to same package": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + UpstreamRef: &api.PackageRevisionRef{ + Name: "foo-12345", + }, + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeClone, + Clone: &api.PackageCloneTaskSpec{ + Upstream: api.UpstreamPackage{ + UpstreamRef: &api.PackageRevisionRef{ + Name: "foo-67890", + }, + }, + }, + }, + ), + repoRevs: []repository.PackageRevision{ + &fake.PackageRevision{ + Name: "foo-12345", + PackageRevisionKey: repository.PackageRevisionKey{ + Repository: "foo", + Package: "pkg", + }, + }, + &fake.PackageRevision{ + Name: "foo-67890", + PackageRevisionKey: repository.PackageRevisionKey{ + Repository: "foo", + Package: "pkg", + }, + }, + }, + hasSameOrigin: true, + expectedErr: nil, + }, + "edit task, existing packagerevision": { + obj: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{ + { + Type: api.TaskTypeEdit, + Edit: &api.PackageEditTaskSpec{ + Source: &api.PackageRevisionRef{ + Name: "foo-12345", + }, + }, + }, + }, + }, + }, + existingRevs: toFakeRepoPkgRevSlice( + api.Task{ + Type: api.TaskTypeInit, + Init: &api.PackageInitTaskSpec{}, + }, + ), + hasSameOrigin: true, + expectedErr: nil, + }, + } + + for tn := range testCases { + tc := testCases[tn] + t.Run(tn, func(t *testing.T) { + ctx := context.TODO() + repo := &fake.Repository{ + PackageRevisions: tc.repoRevs, + } + sameOrigin, err := ensureSameOrigin(ctx, repo, tc.obj, tc.existingRevs) + if tc.expectedErr != nil { + if err == nil { + t.Error("expected error, but didn't get one") + } + if err != tc.expectedErr { + t.Errorf("expected error %v, but got %v", tc.expectedErr, err) + } + } + + if sameOrigin != tc.hasSameOrigin { + t.Errorf("expected sameOrigin %t, but got %t", tc.hasSameOrigin, sameOrigin) + } + }) + } +} + +func toFakeRepoPkgRevSlice(task api.Task) []repository.PackageRevision { + return []repository.PackageRevision{ + &fake.PackageRevision{ + PackageRevision: &api.PackageRevision{ + Spec: api.PackageRevisionSpec{ + Tasks: []api.Task{task}, + }, + }, + }, + } +} diff --git a/porch/pkg/engine/fake/repository.go b/porch/pkg/engine/fake/repository.go index cb56d49567..15efad0319 100644 --- a/porch/pkg/engine/fake/repository.go +++ b/porch/pkg/engine/fake/repository.go @@ -30,8 +30,23 @@ type Repository struct { var _ repository.Repository = &Repository{} -func (r *Repository) ListPackageRevisions(context.Context, repository.ListPackageRevisionFilter) ([]repository.PackageRevision, error) { - return r.PackageRevisions, nil +func (r *Repository) ListPackageRevisions(_ context.Context, filter repository.ListPackageRevisionFilter) ([]repository.PackageRevision, error) { + var revs []repository.PackageRevision + for _, rev := range r.PackageRevisions { + if filter.KubeObjectName != "" && filter.KubeObjectName == rev.KubeObjectName() { + revs = append(revs, rev) + } + if filter.Package != "" && filter.Package == rev.Key().Package { + revs = append(revs, rev) + } + if filter.Revision != "" && filter.Revision == rev.Key().Revision { + revs = append(revs, rev) + } + if filter.WorkspaceName != "" && filter.WorkspaceName == rev.Key().WorkspaceName { + revs = append(revs, rev) + } + } + return revs, nil } func (r *Repository) CreatePackageRevision(_ context.Context, pr *v1alpha1.PackageRevision) (repository.PackageDraft, error) { diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go index c5b638db44..bf14383769 100644 --- a/porch/test/e2e/e2e_test.go +++ b/porch/test/e2e/e2e_test.go @@ -489,6 +489,118 @@ func (t *PorchSuite) TestCloneIntoDeploymentRepository(ctx context.Context) { } } +func (t *PorchSuite) TestEditPackageRevision(ctx context.Context) { + const ( + repository = "edit-test" + packageName = "simple-package" + otherPackageName = "other-package" + workspace = "workspace" + workspace2 = "workspace2" + ) + + t.registerMainGitRepositoryF(ctx, repository) + + // Create a new package (via init) + pr := &porchapi.PackageRevision{ + TypeMeta: metav1.TypeMeta{ + Kind: "PackageRevision", + APIVersion: porchapi.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + }, + Spec: porchapi.PackageRevisionSpec{ + PackageName: packageName, + WorkspaceName: workspace, + RepositoryName: repository, + }, + } + t.CreateF(ctx, pr) + + // Create a new revision, but with a different package as the source. + // This is not allowed. + invalidEditPR := &porchapi.PackageRevision{ + TypeMeta: metav1.TypeMeta{ + Kind: "PackageRevision", + APIVersion: porchapi.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + }, + Spec: porchapi.PackageRevisionSpec{ + PackageName: otherPackageName, + WorkspaceName: workspace2, + RepositoryName: repository, + Tasks: []porchapi.Task{ + { + Type: porchapi.TaskTypeEdit, + Edit: &porchapi.PackageEditTaskSpec{ + Source: &porchapi.PackageRevisionRef{ + Name: pr.Name, + }, + }, + }, + }, + }, + } + if err := t.client.Create(ctx, invalidEditPR); err == nil { + t.Fatalf("Expected error for source revision being from different package") + } + + // Create a new revision of the package with a source that is a revision + // of the same package. + editPR := &porchapi.PackageRevision{ + TypeMeta: metav1.TypeMeta{ + Kind: "PackageRevision", + APIVersion: porchapi.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: t.namespace, + }, + Spec: porchapi.PackageRevisionSpec{ + PackageName: packageName, + WorkspaceName: workspace2, + RepositoryName: repository, + Tasks: []porchapi.Task{ + { + Type: porchapi.TaskTypeEdit, + Edit: &porchapi.PackageEditTaskSpec{ + Source: &porchapi.PackageRevisionRef{ + Name: pr.Name, + }, + }, + }, + }, + }, + } + if err := t.client.Create(ctx, editPR); err == nil { + t.Fatalf("Expected error for source revision not being published") + } + + // Publish the source package to make it a valid source for edit. + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(ctx, pr) + + // Approve the package + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + t.UpdateApprovalF(ctx, pr, metav1.UpdateOptions{}) + + // Create a new revision with the edit task. + t.CreateF(ctx, editPR) + + // Check its task list + var pkgRev porchapi.PackageRevision + t.GetF(ctx, client.ObjectKey{ + Namespace: t.namespace, + Name: editPR.Name, + }, &pkgRev) + tasks := pkgRev.Spec.Tasks + for _, tsk := range tasks { + t.Logf("Task: %s", tsk.Type) + } + assert.Equal(t, 2, len(tasks)) +} + // Test will initialize an empty package, update its resources, adding a function // to the Kptfile's pipeline, and then check that the package was re-rendered. func (t *PorchSuite) TestUpdateResources(ctx context.Context) {