Skip to content

Commit b94696b

Browse files
lapentadlapentad
authored
Aligned clone validation between client and server (#330)
* Aligned clone validation between client and server Signed-off-by: lapentafd <francesco.lapenta@est.tech> * removal of replay-strategy Signed-off-by: lapentafd <francesco.lapenta@est.tech> * changes to cli e2e that used replay Signed-off-by: lapentafd <francesco.lapenta@est.tech> --------- Signed-off-by: lapentafd <francesco.lapenta@est.tech>
1 parent ec2b277 commit b94696b

5 files changed

Lines changed: 161 additions & 47 deletions

File tree

‎pkg/cli/commands/rpkg/copy/command.go‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ func newRunner(ctx context.Context, rcg *genericclioptions.ConfigFlags) *runner
5353
Hidden: porch.HidePorchCommands,
5454
}
5555
r.Command.Flags().StringVar(&r.workspace, "workspace", "", "Workspace name of the copy of the package.")
56-
r.Command.Flags().BoolVar(&r.replayStrategy, "replay-strategy", false, "Use replay strategy for creating new package revision.")
5756
return r
5857
}
5958

@@ -65,8 +64,7 @@ type runner struct {
6564

6665
copy porchapi.PackageEditTaskSpec
6766

68-
workspace string // Target package revision workspaceName
69-
replayStrategy bool
67+
workspace string // Target package revision workspaceName
7068
}
7169

7270
func (r *runner) preRunE(_ *cobra.Command, args []string) error {
@@ -135,19 +133,16 @@ func (r *runner) getPackageRevisionSpec() (*porchapi.PackageRevisionSpec, error)
135133
RepositoryName: packageRevision.Spec.RepositoryName,
136134
}
137135

138-
if len(packageRevision.Spec.Tasks) == 0 || !r.replayStrategy {
139-
spec.Tasks = []porchapi.Task{
140-
{
141-
Type: porchapi.TaskTypeEdit,
142-
Edit: &porchapi.PackageEditTaskSpec{
143-
Source: &porchapi.PackageRevisionRef{
144-
Name: packageRevision.Name,
145-
},
136+
spec.Tasks = []porchapi.Task{
137+
{
138+
Type: porchapi.TaskTypeEdit,
139+
Edit: &porchapi.PackageEditTaskSpec{
140+
Source: &porchapi.PackageRevisionRef{
141+
Name: packageRevision.Name,
146142
},
147143
},
148-
}
149-
} else {
150-
spec.Tasks = packageRevision.Spec.Tasks
144+
},
151145
}
146+
152147
return spec, nil
153148
}

‎pkg/engine/engine.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,12 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *
164164
return nil, err
165165
}
166166

167+
if newPr.Spec.Tasks[0].Type == api.TaskTypeClone {
168+
if err := validateCloneTask(newPr, revs); err != nil {
169+
return nil, err
170+
}
171+
}
172+
167173
if newPr.Spec.Tasks[0].Type == api.TaskTypeUpgrade {
168174
if err := validateUpgradeTask(ctx, revs, newPr.Spec.Tasks[0].Upgrade); err != nil {
169175
return nil, err
@@ -243,6 +249,18 @@ func ensureUniqueWorkspaceName(obj *api.PackageRevision, existingRevs []reposito
243249
return nil
244250
}
245251

252+
// validateCloneTask returns an error if the package already exists in the repository
253+
func validateCloneTask(obj *api.PackageRevision, existingRevs []repository.PackageRevision) error {
254+
for _, r := range existingRevs {
255+
k := r.Key()
256+
if k.PkgKey.RepoKey.Name == obj.Spec.RepositoryName && k.PkgKey.Package == obj.Spec.PackageName {
257+
return fmt.Errorf("`clone` cannot create a new revision for package %q that already exists in repo %q; make subsequent revisions using `copy`",
258+
obj.Spec.PackageName, obj.Spec.RepositoryName)
259+
}
260+
}
261+
return nil
262+
}
263+
246264
func (cad *cadEngine) UpdatePackageRevision(ctx context.Context, version int, repositoryObj *configapi.Repository, repoPr repository.PackageRevision, oldObj, newObj *api.PackageRevision, parent repository.PackageRevision) (repository.PackageRevision, error) {
247265
ctx, span := tracer.Start(ctx, "cadEngine::UpdatePackageRevision", trace.WithAttributes())
248266
defer span.End()

‎pkg/engine/engine_test.go‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,3 +400,103 @@ func TestValidateUpgradeTask(t *testing.T) {
400400
assert.ErrorContains(t, err, local.KubeObjectName())
401401
})
402402
}
403+
404+
func TestCreateCloneTaskValidation(t *testing.T) {
405+
tests := []struct {
406+
name string
407+
existingRevs []repository.PackageRevision
408+
expectedError bool
409+
errorContains string
410+
}{
411+
{
412+
name: "success - no existing revisions",
413+
existingRevs: []repository.PackageRevision{},
414+
expectedError: false,
415+
},
416+
{
417+
name: "success - existing revision in different repo",
418+
existingRevs: []repository.PackageRevision{
419+
&fake.FakePackageRevision{
420+
PrKey: repository.PackageRevisionKey{
421+
PkgKey: repository.PackageKey{
422+
RepoKey: repository.RepositoryKey{
423+
Name: "different-repo",
424+
},
425+
Package: "test-package",
426+
},
427+
WorkspaceName: "v1",
428+
},
429+
},
430+
},
431+
expectedError: false,
432+
},
433+
{
434+
name: "failure - existing revision with same package and repo",
435+
existingRevs: []repository.PackageRevision{
436+
&fake.FakePackageRevision{
437+
PrKey: repository.PackageRevisionKey{
438+
PkgKey: repository.PackageKey{
439+
RepoKey: repository.RepositoryKey{
440+
Name: "test-repo",
441+
},
442+
Package: "test-package",
443+
},
444+
WorkspaceName: "v1",
445+
},
446+
},
447+
},
448+
expectedError: true,
449+
errorContains: "`clone` cannot create a new revision for package \"test-package\" that already exists in repo \"test-repo\"",
450+
},
451+
}
452+
453+
for _, tt := range tests {
454+
t.Run(tt.name, func(t *testing.T) {
455+
f := newTestFixture(t)
456+
mockPkgRev := setupMockPackageRevision(t)
457+
mockDraft := &mockrepo.MockPackageRevisionDraft{}
458+
459+
// Create a package revision with CLONE task
460+
f.packageRevision.Spec.Tasks = []porchapi.Task{
461+
{
462+
Type: porchapi.TaskTypeClone,
463+
Clone: &porchapi.PackageCloneTaskSpec{
464+
Strategy: porchapi.ResourceMerge,
465+
Upstream: porchapi.UpstreamPackage{
466+
Type: porchapi.RepositoryTypeGit,
467+
Git: &porchapi.GitPackage{
468+
Repo: "https://example.com/repo",
469+
Ref: "main",
470+
Directory: "/",
471+
},
472+
},
473+
},
474+
},
475+
}
476+
477+
// Setup mocks
478+
mockDraft.On("UpdateResources", mock.Anything, mock.Anything, mock.Anything).Return(nil)
479+
mockDraft.On("UpdateLifecycle", mock.Anything, mock.Anything).Return(nil)
480+
481+
f.mockRepo.On("ListPackageRevisions", mock.Anything, mock.Anything).Return(tt.existingRevs, nil)
482+
f.mockRepo.On("CreatePackageRevisionDraft", mock.Anything, mock.Anything).Return(mockDraft, nil).Maybe()
483+
f.mockRepo.On("ClosePackageRevisionDraft", mock.Anything, mock.Anything, mock.Anything).Return(mockPkgRev, nil).Maybe()
484+
f.mockRepo.On("Close", mock.Anything).Return(nil).Maybe()
485+
f.mockRepo.On("Key", mock.Anything).Return(repository.RepositoryKey{}).Maybe()
486+
487+
f.mockTaskHandler.On("ApplyTask", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe()
488+
489+
_, err := f.engine.CreatePackageRevision(context.Background(), f.repositoryObj, f.packageRevision, nil)
490+
491+
if tt.expectedError {
492+
assert.Error(t, err)
493+
assert.Contains(t, err.Error(), tt.errorContains)
494+
} else {
495+
assert.NoError(t, err)
496+
}
497+
498+
f.mockRepo.Close(context.Background())
499+
f.mockRepo.AssertExpectations(t)
500+
})
501+
}
502+
}

‎test/e2e/cli/testdata/rpkg-unready/config.yaml‎

Lines changed: 33 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -19,38 +19,13 @@ commands:
1919
- unready-edit
2020
stdout: |
2121
git.unready-edit.clone-1 created
22-
- args:
23-
- porchctl
24-
- rpkg
25-
- copy
26-
- --namespace=rpkg-unready
27-
- --workspace=copy-2
28-
- --replay-strategy=true
29-
- git.unready-edit.clone-1
30-
stdout: "git.unready-edit.copy-2 created\n"
31-
- args:
32-
- porchctl
33-
- rpkg
34-
- propose
35-
- --namespace=rpkg-unready
36-
- git.unready-edit.copy-2
37-
stderr: "git.unready-edit.copy-2 failed (readiness conditions not met)\nError: errors:\n readiness conditions not met \n"
38-
exitCode: 1
39-
- args:
40-
- porchctl
41-
- rpkg
42-
- approve
43-
- --namespace=rpkg-unready
44-
- git.unready-edit.copy-2
45-
stderr: "git.unready-edit.copy-2 failed (readiness conditions not met)\nError: errors:\n readiness conditions not met \n"
46-
exitCode: 1
4722
- args:
4823
- porchctl
4924
- rpkg
5025
- pull
5126
- --namespace=rpkg-unready
52-
- git.unready-edit.copy-2
53-
- /tmp/porch-e2e/pkg-unready-git.unready-edit.copy-2
27+
- git.unready-edit.clone-1
28+
- /tmp/porch-e2e/pkg-unready-git.unready-edit.clone-1
5429
- args:
5530
- kpt
5631
- fn
@@ -59,7 +34,7 @@ commands:
5934
- ghcr.io/kptdev/krm-functions-catalog/search-replace:v0.2.0
6035
- --match-kind
6136
- Kptfile
62-
- /tmp/porch-e2e/pkg-unready-git.unready-edit.copy-2
37+
- /tmp/porch-e2e/pkg-unready-git.unready-edit.clone-1
6338
- --
6439
- by-path=status.conditions[0].status
6540
- put-value=True
@@ -69,10 +44,37 @@ commands:
6944
- rpkg
7045
- push
7146
- --namespace=rpkg-unready
72-
- git.unready-edit.copy-2
73-
- /tmp/porch-e2e/pkg-unready-git.unready-edit.copy-2
47+
- git.unready-edit.clone-1
48+
- /tmp/porch-e2e/pkg-unready-git.unready-edit.clone-1
7449
stdout: |
75-
git.unready-edit.copy-2 pushed
50+
git.unready-edit.clone-1 pushed
51+
- args:
52+
- porchctl
53+
- rpkg
54+
- propose
55+
- --namespace=rpkg-unready
56+
- git.unready-edit.clone-1
57+
stdout: |
58+
git.unready-edit.clone-1 proposed
59+
exitCode: 0
60+
- args:
61+
- porchctl
62+
- rpkg
63+
- approve
64+
- --namespace=rpkg-unready
65+
- git.unready-edit.clone-1
66+
stdout: |
67+
git.unready-edit.clone-1 approved
68+
exitCode: 0
69+
- args:
70+
- porchctl
71+
- rpkg
72+
- copy
73+
- --namespace=rpkg-unready
74+
- --workspace=copy-2
75+
- git.unready-edit.clone-1
76+
stdout: "git.unready-edit.copy-2 created\n"
77+
exitCode: 0
7678
- args:
7779
- porchctl
7880
- rpkg
@@ -91,4 +93,3 @@ commands:
9193
stdout: |
9294
git.unready-edit.copy-2 approved
9395
exitCode: 0
94-

‎test/e2e/e2e_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2092,7 +2092,7 @@ func (t *PorchSuite) TestPodEvaluator() {
20922092
Namespace: t.Namespace,
20932093
},
20942094
Spec: porchapi.PackageRevisionSpec{
2095-
PackageName: "test-fn-pod-hierarchy",
2095+
PackageName: "test-fn-pod-hierarchy-2",
20962096
WorkspaceName: "workspace-2",
20972097
RepositoryName: "git-fn-pod",
20982098
Tasks: []porchapi.Task{

0 commit comments

Comments
 (0)