diff --git a/api/sql/porch-db-1.6.0-1.6.4.sql b/api/sql/porch-db-1.6.0-1.6.4.sql new file mode 100644 index 000000000..a11ccb9dd --- /dev/null +++ b/api/sql/porch-db-1.6.0-1.6.4.sql @@ -0,0 +1,18 @@ +/* +Copyright 2026 The kpt Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +ALTER TABLE package_revisions + ADD COLUMN IF NOT EXISTS last_pushed_db_updated TIMESTAMP; diff --git a/api/sql/porch-db-1.6.4-1.6.0.sql b/api/sql/porch-db-1.6.4-1.6.0.sql new file mode 100644 index 000000000..7412d3645 --- /dev/null +++ b/api/sql/porch-db-1.6.4-1.6.0.sql @@ -0,0 +1,18 @@ +/* +Copyright 2026 The kpt Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +ALTER TABLE package_revisions + DROP COLUMN IF EXISTS last_pushed_db_updated; diff --git a/api/sql/porch-db.sql b/api/sql/porch-db.sql index e19d9ab63..cd3c9391c 100644 --- a/api/sql/porch-db.sql +++ b/api/sql/porch-db.sql @@ -88,6 +88,7 @@ CREATE TABLE IF NOT EXISTS package_revisions ( kptfile_status TEXT NOT NULL DEFAULT '{}', resources_size BIGINT NOT NULL DEFAULT 0, upstream_ref_name TEXT NOT NULL DEFAULT '', + last_pushed_db_updated TIMESTAMP, PRIMARY KEY (k8s_name_space, k8s_name), CONSTRAINT fk_package FOREIGN KEY (k8s_name_space, package_k8s_name) diff --git a/deployments/porch/3-porch-postgres-bundle.yaml b/deployments/porch/3-porch-postgres-bundle.yaml index 2306da015..7a41ae95c 100644 --- a/deployments/porch/3-porch-postgres-bundle.yaml +++ b/deployments/porch/3-porch-postgres-bundle.yaml @@ -356,6 +356,7 @@ data: kptfile_status TEXT NOT NULL DEFAULT '{}', resources_size BIGINT NOT NULL DEFAULT 0, upstream_ref_name TEXT NOT NULL DEFAULT '', + last_pushed_db_updated TIMESTAMP, PRIMARY KEY (k8s_name_space, k8s_name), CONSTRAINT fk_package FOREIGN KEY (k8s_name_space, package_k8s_name) diff --git a/deployments/porch/9-controllers.yaml b/deployments/porch/9-controllers.yaml index 2859e4faa..8c7d9be45 100644 --- a/deployments/porch/9-controllers.yaml +++ b/deployments/porch/9-controllers.yaml @@ -51,6 +51,7 @@ spec: imagePullPolicy: IfNotPresent args: - --repositories.cache-type=DB + - --repositories.push-drafts-to-git=false - --repositories.create-v1alpha2-rpkg=false securityContext: runAsNonRoot: true diff --git a/make/deploy.mk b/make/deploy.mk index c3a4127c7..742942c98 100644 --- a/make/deploy.mk +++ b/make/deploy.mk @@ -29,6 +29,9 @@ export FN_RUNNER_WARM_UP_POD_CACHE ?= true # Enable v1alpha2 PackageRevision support (CRD install + controller flag + reconciler) export CREATE_V1ALPHA2_RPKG ?= false +# Push draft & proposed PR's to git rather than DB only +export DB_PUSH_DRAFTS_TO_GIT ?= false + # Reconciler configuration ALL_RECONCILERS=packagevariants,packagevariantsets,repositories ifndef RECONCILERS @@ -52,6 +55,7 @@ run-in-kind: load-images-to-kind deployment-config deploy-current-config run-in-kind-v1alpha2: IMAGE_REPO=porch-kind## Build and deploy porch into a kind cluster with DB cache and v1alpha2 PackageRevision CRD creation run-in-kind-v1alpha2: PORCH_CACHE_TYPE=DB run-in-kind-v1alpha2: CREATE_V1ALPHA2_RPKG=true +run-in-kind-v1alpha2: DB_PUSH_DRAFTS_TO_GIT=true run-in-kind-v1alpha2: load-images-to-kind deployment-config deploy-current-config .PHONY: run-in-kind-v1alpha2-no-controller diff --git a/pkg/cache/dbcache/dbpackagerevision.go b/pkg/cache/dbcache/dbpackagerevision.go index 5868a88f1..cbc19747b 100644 --- a/pkg/cache/dbcache/dbpackagerevision.go +++ b/pkg/cache/dbcache/dbpackagerevision.go @@ -26,7 +26,6 @@ import ( "github.com/kptdev/kpt/pkg/kptfile/kptfileutil" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" cachetypes "github.com/kptdev/porch/pkg/cache/types" - "github.com/kptdev/porch/pkg/engine" "github.com/kptdev/porch/pkg/repository" "github.com/kptdev/porch/pkg/util" pctx "github.com/kptdev/porch/pkg/util/context" @@ -76,27 +75,22 @@ func extractFromKptfile(resources map[string]string) (kptfileStatus, []porchapi. } type dbPackageRevision struct { - repo *dbRepository - pkgRevKey repository.PackageRevisionKey - meta metav1.ObjectMeta - spec *porchapi.PackageRevisionSpec - updated time.Time - updatedBy string - lifecycle porchapi.PackageRevisionLifecycle - extPRID kptfile.Locator - latest bool - deployment bool - tasks []porchapi.Task - resources map[string]string - resourcesDirty bool - kptfileStatus kptfileStatus - resourcesSizeBytes int64 - - // gitPRDraft maintains the draft in the external git repository during editing (when pushDraftsToGit is true) - gitPRDraft repository.PackageRevisionDraft - - // gitPR is the closed package revision in git (when pushDraftsToGit is true) - gitPR repository.PackageRevision + repo *dbRepository + pkgRevKey repository.PackageRevisionKey + meta metav1.ObjectMeta + spec *porchapi.PackageRevisionSpec + updated time.Time + updatedBy string + lifecycle porchapi.PackageRevisionLifecycle + extPRID kptfile.Locator + latest bool + deployment bool + tasks []porchapi.Task + resources map[string]string + resourcesDirty bool + kptfileStatus kptfileStatus + resourcesSizeBytes int64 + lastPushedDbUpdated *time.Time } // ensureRepo resolves the repository from the cache if pr.repo is nil. @@ -158,8 +152,9 @@ func (pr *dbPackageRevision) savePackageRevision(ctx context.Context, saveResour pr.updatedBy = getCurrentUser() } - _, err := pkgRevReadFromDB(ctx, pr.Key(), false) + existing, err := pkgRevReadFromDB(ctx, pr.Key(), false) if err == nil { + preservePushMarkersIfUnset(pr, existing) updErr := pkgRevUpdateDB(ctx, pr, saveResources) if updErr == nil && saveResources { sent := pr.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Modified, pr) @@ -202,6 +197,9 @@ func (pr *dbPackageRevision) UpdateLifecycle(ctx context.Context, newLifecycle p _, span := tracer.Start(ctx, "dbPackageRevision::UpdateLifecycle", trace.WithAttributes()) defer span.End() + lockPkgKey(pr.pkgRevKey.PkgKey) + defer unlockPkgKey(pr.pkgRevKey.PkgKey) + if err := pr.ensureRepo(); err != nil { return fmt.Errorf("cannot update lifecycle for package revision %s: %w", pr.KubeObjectName(), err) } @@ -214,11 +212,6 @@ func (pr *dbPackageRevision) UpdateLifecycle(ctx context.Context, newLifecycle p klog.V(3).InfoS("[DB Cache] Lifecycle updated in database and pushed to external repo for PackageRevision", pctx.LogMetadataFrom(ctx)...) }() - } else if pr.repo.pushDraftsToGit && pr.gitPRDraft != nil { - klog.InfoS("[DB Cache] Updating lifecycle in database and in Git draft for PackageRevision", pctx.LogMetadataFrom(ctx)...) - defer func() { - klog.V(3).InfoS("[DB Cache] Lifecycle updated in database and in Git draft for PackageRevision", pctx.LogMetadataFrom(ctx)...) - }() } else { klog.InfoS("[DB Cache] Updating lifecycle in database for PackageRevision", pctx.LogMetadataFrom(ctx)...) defer func() { @@ -231,20 +224,12 @@ func (pr *dbPackageRevision) UpdateLifecycle(ctx context.Context, newLifecycle p pr.pkgRevKey.Revision = 0 return pkgerrors.Wrapf(err, "dbPackageRevision:UpdateLifecycle: could not publish package revision %+v", pr.Key()) } - // drops cached stale draft so it doesnt trigger closure - pr.gitPRDraft = nil } else if porchapi.LifecycleIsPublished(pr.lifecycle) { return pr.updateLifecycleOnPublishedPR(ctx, newLifecycle) } pr.lifecycle = newLifecycle - if pr.repo.pushDraftsToGit && pr.gitPRDraft != nil { - if err := pr.gitPRDraft.UpdateLifecycle(ctx, newLifecycle); err != nil { - klog.Warningf("failed to update git draft lifecycle for %+v: %v", pr.Key(), err) - } - } - return nil } @@ -408,7 +393,17 @@ func (pr *dbPackageRevision) SetMeta(ctx context.Context, meta metav1.ObjectMeta _, span := tracer.Start(ctx, "dbPackageRevision::SetMeta", trace.WithAttributes()) defer span.End() + lockPkgKey(pr.pkgRevKey.PkgKey) + defer unlockPkgKey(pr.pkgRevKey.PkgKey) + pr.meta = meta + + if existing, err := pkgRevReadFromDB(ctx, pr.Key(), false); err == nil { + preservePushMarkersIfUnset(pr, existing) + } else if err != sql.ErrNoRows { + return err + } + return pkgRevUpdateDB(ctx, pr, false) } @@ -482,6 +477,17 @@ func (pr *dbPackageRevision) copyToThis(otherPr *dbPackageRevision) { pr.tasks = otherPr.tasks pr.resources = otherPr.resources pr.resourcesSizeBytes = otherPr.resourcesSizeBytes + pr.lastPushedDbUpdated = otherPr.lastPushedDbUpdated +} + +func preservePushMarkersIfUnset(pr, existing *dbPackageRevision) { + if pr.lastPushedDbUpdated != nil || existing.lastPushedDbUpdated == nil { + return + } + pr.lastPushedDbUpdated = existing.lastPushedDbUpdated + if extPRCommit(pr) == unpushedGitCommit { + pr.extPRID = existing.extPRID + } } func (pr *dbPackageRevision) UpdateResources(ctx context.Context, new *porchapi.PackageRevisionResources, change *porchapi.Task) error { @@ -492,18 +498,6 @@ func (pr *dbPackageRevision) UpdateResources(ctx context.Context, new *porchapi. return fmt.Errorf("cannot update resources for package revision %s: %w", pr.KubeObjectName(), err) } - if pr.repo.pushDraftsToGit && pr.gitPRDraft != nil { - klog.InfoS("[DB Cache] Updating resources in memory and in Git draft for PackageRevision", pctx.LogMetadataFrom(ctx)...) - defer func() { - klog.V(3).InfoS("[DB Cache] Resources updated in memory and in Git draft for PackageRevision", pctx.LogMetadataFrom(ctx)...) - }() - } else { - klog.InfoS("[DB Cache] Updating resources in memory for PackageRevision", pctx.LogMetadataFrom(ctx)...) - defer func() { - klog.V(3).InfoS("[DB Cache] Resources updated in memory for PackageRevision", pctx.LogMetadataFrom(ctx)...) - }() - } - pr.resources = new.Spec.Resources pr.resourcesDirty = true status, gates, pkgMeta := extractFromKptfile(pr.resources) @@ -523,12 +517,6 @@ func (pr *dbPackageRevision) UpdateResources(ctx context.Context, new *porchapi. pr.tasks = []porchapi.Task{*change} } - if pr.repo.pushDraftsToGit && pr.gitPRDraft != nil { - if err := pr.gitPRDraft.UpdateResources(ctx, new, change); err != nil { - klog.Warningf("failed to update git draft resources for %+v: %v", pr.Key(), err) - } - } - return nil } @@ -544,16 +532,7 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap pr.pkgRevKey.Revision = latestRev + 1 pr.lifecycle = newLifecycle - var gitPR repository.PackageRevision - if pr.repo.pushDraftsToGit { - if pr.gitPR != nil { - gitPR = pr.gitPR - } else { - gitPR = pr.repo.getCachedGitPR(pr.Key().PkgKey, pr.Key().WorkspaceName) - } - } - - pushedPRExtID, err := engine.PushPackageRevision(ctx, pr.repo.externalRepo, pr, pr.repo.pushDraftsToGit, gitPR) + pushedPRExtID, err := PushPublishedPackageRevision(ctx, pr.repo.externalRepo, pr, pr.repo.pushDraftsToGit, hasBeenPushedToGit(pr)) if err != nil { klog.Warningf("push of package revision %+v to external repo failed, %q", pr.Key(), err) pr.pkgRevKey.Revision = 0 @@ -562,6 +541,8 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap } pr.extPRID = pushedPRExtID + dbUpdated := pr.updated + pr.lastPushedDbUpdated = &dbUpdated if err = pkgRevUpdateDB(ctx, pr, false); err != nil { return pkgerrors.Wrapf(err, "dbPackageRevision:publishPR: failed to save package revision %+v to database after push to external repo", pr.Key()) diff --git a/pkg/cache/dbcache/dbpackagerevision_test.go b/pkg/cache/dbcache/dbpackagerevision_test.go index a5cfece64..c3eb0f0aa 100644 --- a/pkg/cache/dbcache/dbpackagerevision_test.go +++ b/pkg/cache/dbcache/dbpackagerevision_test.go @@ -770,13 +770,6 @@ func (t *DbTestSuite) TestDBPackageRevisionPublishWithPushDraftsToGit() { testRepo.externalRepo = extRepo testRepo.pushDraftsToGit = true - testRepo.gitPRCache = make(map[string]repository.PackageRevision) - - // Create: a git draft is opened and then closed, yielding the cached git PR. - initialGitDraft := mockrepo.NewMockPackageRevisionDraft(t.T()) - cachedGitPR := mockrepo.NewMockPackageRevision(t.T()) - extRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(initialGitDraft, nil).Once() - extRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, initialGitDraft, 0).Return(cachedGitPR, nil).Once() newPRDef := porchapi.PackageRevision{ Spec: porchapi.PackageRevisionSpec{ @@ -792,7 +785,6 @@ func (t *DbTestSuite) TestDBPackageRevisionPublishWithPushDraftsToGit() { dbPR, err := testRepo.ClosePackageRevisionDraft(ctx, prDraft, 0) t.Require().NoError(err) - t.Require().Nil(dbPR.(*dbPackageRevision).gitPRDraft, "closing a draft must release the git draft handle") // Propose. err = dbPR.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecycleProposed) @@ -801,30 +793,20 @@ func (t *DbTestSuite) TestDBPackageRevisionPublishWithPushDraftsToGit() { dbPR, err = testRepo.ClosePackageRevisionDraft(ctx, dbPR.(repository.PackageRevisionDraft), 0) t.Require().NoError(err) - // Approve. dbRepository.UpdatePackageRevision reopens the cached git PR as a draft, and - // publishPR opens and closes a second one with the real revision number. - staleGitDraft := mockrepo.NewMockPackageRevisionDraft(t.T()) + // Publish pushes to git via PushPublishedPackageRevision (no prior draft push, so a new git draft is created). publishGitDraft := mockrepo.NewMockPackageRevisionDraft(t.T()) publishedGitPR := mockrepo.NewMockPackageRevision(t.T()) - extRepo.EXPECT().UpdatePackageRevision(mock.Anything, cachedGitPR).Return(staleGitDraft, nil).Once() - extRepo.EXPECT().UpdatePackageRevision(mock.Anything, cachedGitPR).Return(publishGitDraft, nil).Once() + extRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(publishGitDraft, nil).Once() + publishGitDraft.EXPECT().UpdateResources(mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() publishGitDraft.EXPECT().UpdateLifecycle(mock.Anything, porchapi.PackageRevisionLifecyclePublished).Return(nil).Once() - // The revision number, never 0, is what reaches git for a Published draft. extRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, publishGitDraft, 1).Return(publishedGitPR, nil).Once() publishedGitPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, nil).Once() - approveDraft, err := testRepo.UpdatePackageRevision(ctx, dbPR) - t.Require().NoError(err) - t.Require().Equal(staleGitDraft, approveDraft.(*dbPackageRevision).gitPRDraft) - - err = approveDraft.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecyclePublished) + err = dbPR.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecyclePublished) t.Require().NoError(err) - t.Require().Nil(approveDraft.(*dbPackageRevision).gitPRDraft, - "publishing must release the stale git draft handle so it is not closed with version 0") - // No further git calls are expected here: staleGitDraft is never closed. - publishedPR, err := testRepo.ClosePackageRevisionDraft(ctx, approveDraft, 0) + publishedPR, err := testRepo.ClosePackageRevisionDraft(ctx, dbPR.(repository.PackageRevisionDraft), 0) t.Require().NoError(err) t.Require().Equal(1, publishedPR.Key().Revision) t.Require().Equal(porchapi.PackageRevisionLifecyclePublished, publishedPR.Lifecycle(ctx)) diff --git a/pkg/cache/dbcache/dbpackagerevisionsql.go b/pkg/cache/dbcache/dbpackagerevisionsql.go index 9610281b3..0f18983e1 100644 --- a/pkg/cache/dbcache/dbpackagerevisionsql.go +++ b/pkg/cache/dbcache/dbpackagerevisionsql.go @@ -18,6 +18,7 @@ import ( "context" "database/sql" "fmt" + "time" kptfile "github.com/kptdev/kpt/api/kptfile/v1" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" @@ -53,7 +54,8 @@ func pkgRevReadFromDB(ctx context.Context, prk repository.PackageRevisionKey, re package_revisions.latest, package_revisions.tasks, package_revisions.kptfile_status, - package_revisions.resources_size + package_revisions.resources_size, + package_revisions.last_pushed_db_updated FROM package_revisions INNER JOIN packages ON package_revisions.k8s_name_space=packages.k8s_name_space AND package_revisions.package_k8s_name=packages.k8s_name INNER JOIN repositories @@ -126,7 +128,8 @@ func pkgRevListPRsFromDB(ctx context.Context, filter repository.ListPackageRevis package_revisions.latest, package_revisions.tasks, package_revisions.kptfile_status, - package_revisions.resources_size + package_revisions.resources_size, + package_revisions.last_pushed_db_updated FROM package_revisions INNER JOIN packages ON package_revisions.k8s_name_space=packages.k8s_name_space AND package_revisions.package_k8s_name=packages.k8s_name @@ -176,7 +179,8 @@ func pkgRevReadPRsFromDB(ctx context.Context, pk repository.PackageKey) ([]*dbPa package_revisions.latest, package_revisions.tasks, package_revisions.kptfile_status, - package_revisions.resources_size + package_revisions.resources_size, + package_revisions.last_pushed_db_updated FROM package_revisions INNER JOIN packages ON package_revisions.k8s_name_space=packages.k8s_name_space AND package_revisions.package_k8s_name=packages.k8s_name INNER JOIN repositories @@ -227,7 +231,8 @@ func pkgRevReadLatestPRFromDB(ctx context.Context, pk repository.PackageKey) (*d package_revisions.latest, package_revisions.tasks, package_revisions.kptfile_status, - package_revisions.resources_size + package_revisions.resources_size, + package_revisions.last_pushed_db_updated FROM package_revisions INNER JOIN packages ON package_revisions.k8s_name_space=packages.k8s_name_space AND package_revisions.package_k8s_name=packages.k8s_name INNER JOIN repositories @@ -295,6 +300,7 @@ func pkgRevScanRowsFromDB(ctx context.Context, rows *sql.Rows) ([]*dbPackageRevi for rows.Next() { var pkgRev dbPackageRevision var pkgK8SName, prK8SName, metaAsJSON, specAsJSON, extPRID, tasks, kptfileStatusJSON string + var lastPushedDbUpdated sql.NullTime err := rows.Scan( &pkgRev.pkgRevKey.PkgKey.RepoKey.Namespace, @@ -315,13 +321,19 @@ func pkgRevScanRowsFromDB(ctx context.Context, rows *sql.Rows) ([]*dbPackageRevi &pkgRev.latest, &tasks, &kptfileStatusJSON, - &pkgRev.resourcesSizeBytes) + &pkgRev.resourcesSizeBytes, + &lastPushedDbUpdated) if err != nil { klog.Warningf("pkgRevScanRowsFromDB: scanning rows failed: %q", err) return nil, err } + if lastPushedDbUpdated.Valid { + dbUpdated := lastPushedDbUpdated.Time + pkgRev.lastPushedDbUpdated = &dbUpdated + } + repo := cachetypes.CacheInstance.GetRepository(pkgRev.pkgRevKey.PkgKey.RepoKey) if repo != nil { if dbRepo, ok := repo.(*dbRepository); ok { @@ -351,16 +363,19 @@ func pkgRevWriteToDB(ctx context.Context, pr *dbPackageRevision) error { klog.V(5).Infof("pkgRevWriteToDB: writing package revision %+v", pr.Key()) sqlStatement := ` - INSERT INTO package_revisions (k8s_name_space, k8s_name, package_k8s_name, revision, meta, spec, updated, updatedby, lifecycle, ext_pr_id, tasks, kptfile_status, resources_size, upstream_ref_name) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) + INSERT INTO package_revisions (k8s_name_space, k8s_name, package_k8s_name, revision, meta, spec, updated, updatedby, lifecycle, ext_pr_id, tasks, kptfile_status, resources_size, upstream_ref_name, last_pushed_db_updated) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15) ` klog.V(6).Infof("pkgRevWriteToDB: running query %q on package revision %+v", sqlStatement, pr) + + lastPushedDbUpdated := lastPushedDbUpdatedAsNullTime(pr) + prk := pr.Key() if _, err := GetDB().db.Exec(ctx, sqlStatement, prk.K8SNS(), prk.K8SName(), - prk.PKey().K8SName(), prk.Revision, valueAsJSON(pr.meta), valueAsJSON(pr.spec), pr.updated, pr.updatedBy, pr.lifecycle, valueAsJSON(pr.extPRID), valueAsJSON(pr.tasks), valueAsJSON(pr.kptfileStatus), pr.resourcesSizeBytes, extractUpstreamRefName(pr.tasks)); err == nil { + prk.PKey().K8SName(), prk.Revision, valueAsJSON(pr.meta), valueAsJSON(pr.spec), pr.updated, pr.updatedBy, pr.lifecycle, valueAsJSON(pr.extPRID), valueAsJSON(pr.tasks), valueAsJSON(pr.kptfileStatus), pr.resourcesSizeBytes, extractUpstreamRefName(pr.tasks), lastPushedDbUpdated); err == nil { klog.V(5).Infof("pkgRevWriteToDB: query succeeded, row created") } else { klog.Warningf("pkgRevWriteToDB: query failed for %+v %q", pr.Key(), err) @@ -383,15 +398,15 @@ func pkgRevUpdateDB(ctx context.Context, pr *dbPackageRevision, updateResources klog.V(5).Infof("pkgRevUpdateDB: updating package revision %+v", pr.Key()) sqlStatement := ` - UPDATE package_revisions SET package_k8s_name=$3, revision=$4, meta=$5, spec=$6, updated=$7, updatedby=$8, lifecycle=$9, ext_pr_id=$10, tasks=$11, kptfile_status=$12, resources_size=$13, upstream_ref_name=$14 + UPDATE package_revisions SET package_k8s_name=$3, revision=$4, meta=$5, spec=$6, updated=$7, updatedby=$8, lifecycle=$9, ext_pr_id=$10, tasks=$11, kptfile_status=$12, resources_size=$13, upstream_ref_name=$14, last_pushed_db_updated=$15 WHERE k8s_name_space=$1 AND k8s_name=$2 ` if pr.pkgRevKey.Revision == -1 { sqlStatement = ` INSERT INTO package_revisions ( - k8s_name_space, k8s_name, package_k8s_name, revision, meta, spec, updated, updatedby, lifecycle, ext_pr_id, tasks, kptfile_status, resources_size, upstream_ref_name + k8s_name_space, k8s_name, package_k8s_name, revision, meta, spec, updated, updatedby, lifecycle, ext_pr_id, tasks, kptfile_status, resources_size, upstream_ref_name, last_pushed_db_updated ) VALUES ( - $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14 + $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15 ) ON CONFLICT (k8s_name_space, k8s_name) DO UPDATE SET @@ -406,16 +421,20 @@ func pkgRevUpdateDB(ctx context.Context, pr *dbPackageRevision, updateResources tasks = EXCLUDED.tasks, kptfile_status = EXCLUDED.kptfile_status, resources_size = EXCLUDED.resources_size, - upstream_ref_name = EXCLUDED.upstream_ref_name; + upstream_ref_name = EXCLUDED.upstream_ref_name, + last_pushed_db_updated = EXCLUDED.last_pushed_db_updated; ` } klog.V(6).Infof("pkgRevUpdateDB: running query %q on package revision %+v", sqlStatement, pr) + + lastPushedDbUpdated := lastPushedDbUpdatedAsNullTime(pr) + prk := pr.Key() result, err := GetDB().db.Exec(ctx, sqlStatement, prk.K8SNS(), prk.K8SName(), - prk.PKey().K8SName(), prk.Revision, valueAsJSON(pr.meta), valueAsJSON(pr.spec), pr.updated, pr.updatedBy, pr.lifecycle, valueAsJSON(pr.extPRID), valueAsJSON(pr.tasks), valueAsJSON(pr.kptfileStatus), pr.resourcesSizeBytes, extractUpstreamRefName(pr.tasks)) + prk.PKey().K8SName(), prk.Revision, valueAsJSON(pr.meta), valueAsJSON(pr.spec), pr.updated, pr.updatedBy, pr.lifecycle, valueAsJSON(pr.extPRID), valueAsJSON(pr.tasks), valueAsJSON(pr.kptfileStatus), pr.resourcesSizeBytes, extractUpstreamRefName(pr.tasks), lastPushedDbUpdated) if err == nil { if rowsAffected, _ := result.RowsAffected(); rowsAffected == 1 { @@ -703,3 +722,31 @@ func backfillUpstreamRefName(ctx context.Context) error { } return nil } + +func lastPushedDbUpdatedAsNullTime(pr *dbPackageRevision) sql.NullTime { + if pr.lastPushedDbUpdated == nil { + return sql.NullTime{} + } + return sql.NullTime{Valid: true, Time: *pr.lastPushedDbUpdated} +} + +// pkgRevSetLastPushedInDB records the pushed ext_pr_id and last_pushed_db_updated for a package +// revision without touching the `updated`/`updatedby` columns. +func pkgRevSetLastPushedInDB(ctx context.Context, prk repository.PackageRevisionKey, extPRID kptfile.Locator, expectedUpdated time.Time) (bool, error) { + _, span := tracer.Start(ctx, "dbpackagerevisionsql::pkgRevSetLastPushedInDB", trace.WithAttributes()) + defer span.End() + + sqlStatement := ` + UPDATE package_revisions SET ext_pr_id=$3, last_pushed_db_updated=$4 + WHERE k8s_name_space=$1 AND k8s_name=$2 AND updated=$4 + ` + + result, err := GetDB().db.Exec(ctx, sqlStatement, prk.K8SNS(), prk.K8SName(), valueAsJSON(extPRID), expectedUpdated) + if err != nil { + klog.Warningf("pkgRevSetLastPushedInDB: query failed for %+v: %q", prk, err) + return false, err + } + + rowsAffected, _ := result.RowsAffected() + return rowsAffected == 1, nil +} diff --git a/pkg/cache/dbcache/dbpushtogit.go b/pkg/cache/dbcache/dbpushtogit.go new file mode 100644 index 000000000..643aca920 --- /dev/null +++ b/pkg/cache/dbcache/dbpushtogit.go @@ -0,0 +1,240 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package dbcache + +import ( + "context" + "database/sql" + "fmt" + + kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/pkg/repository" + pctx "github.com/kptdev/porch/pkg/util/context" + pkgerrors "github.com/pkg/errors" + "go.opentelemetry.io/otel/trace" + "k8s.io/klog/v2" +) + +func PushPublishedPackageRevision(ctx context.Context, repo repository.Repository, pr repository.PackageRevision, pushDraftsToGit, existingGitBranch bool) (kptfilev1.Locator, error) { + ctx, span := tracer.Start(ctx, "PushPackageRevision", trace.WithAttributes()) + defer span.End() + + prName := repository.ComposePkgRevObjName(pr.Key()) + if pushDraftsToGit { + klog.InfoS("[DBCache] Pushing PackageRevision to repository and to Git for PackageRevision", + pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) + } else { + klog.InfoS("[DBCache] Pushing PackageRevision to repository for PackageRevision", + pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) + } + defer func() { + klog.V(3).InfoS("[DBCache] Push PackageRevision to repository completed for PackageRevision", + pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) + }() + + prLifecycle := pr.Lifecycle(ctx) + if prLifecycle != porchapi.PackageRevisionLifecyclePublished { + return kptfilev1.Locator{}, fmt.Errorf("cannot push package revision %+v, package revision lifecycle is %q, it should be \"Published\"", pr.Key(), prLifecycle) + } + + apiPr, err := pr.GetPackageRevision(ctx) + if err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get API definition:", pr.Key(), repo.Key()) + } + + resources, err := pr.GetResources(ctx) + if err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get package revision resources:", pr.Key(), repo.Key()) + } + + var draft repository.PackageRevisionDraft + var foundExisting bool + + if pushDraftsToGit && existingGitBranch { + existingPRs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Key: repository.PackageRevisionKey{ + PkgKey: pr.Key().PkgKey, + WorkspaceName: pr.Key().WorkspaceName, + }, + }) + + if err == nil && len(existingPRs) > 0 { + draft, err = repo.UpdatePackageRevision(ctx, existingPRs[0]) + if err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update existing package revision:", pr.Key(), repo.Key()) + } + + if err = draft.UpdateResources(ctx, resources, commitTaskForPublishedPush(apiPr.Spec.Tasks, true)); err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update package revision resources on existing draft:", pr.Key(), repo.Key()) + } + foundExisting = true + } + } + + if !foundExisting { + draft, err = repo.CreatePackageRevisionDraft(ctx, apiPr) + if err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not create package revision draft:", pr.Key(), repo.Key()) + } + + if err = draft.UpdateResources(ctx, resources, commitTaskForPublishedPush(apiPr.Spec.Tasks, false)); err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update package revision resources:", pr.Key(), repo.Key()) + } + } + + if err = draft.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecyclePublished); err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update package revision draft lifecycle to \"Published\":", pr.Key(), repo.Key()) + } + + pushedPR, err := repo.ClosePackageRevisionDraft(ctx, draft, pr.Key().Revision) + if err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not close package revision draft:", pr.Key(), repo.Key()) + } + + _, pushedPRUpstreamLock, err := pushedPR.GetLock(ctx) + if err != nil { + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "read of upstream lock for package revision %+v pushed to repository %+v failed", pr.Key(), repo.Key()) + } + + return pushedPRUpstreamLock, nil +} + +func PushDraftPackageRevision(ctx context.Context, repoKey repository.RepositoryKey, pr *dbPackageRevision) { + prKey := pr.Key() + klog.Infof("PushDraftPackageRevision: repo %+v started for %+v", repoKey, prKey) + + lockPkgKey(prKey.PKey()) + defer unlockPkgKey(prKey.PKey()) + + freshPR, err := pkgRevReadFromDB(ctx, prKey, true) + if err != nil { + if err == sql.ErrNoRows { + klog.Infof("PushDraftPackageRevision: repo %+v: PR %+v no longer exists in the database, skipping push", repoKey, prKey) + return + } + klog.Warningf("PushDraftPackageRevision: repo %+v: failed to re-read PR %+v from database: %v", repoKey, prKey, err) + return + } + pr = freshPR + + if pr.lifecycle != porchapi.PackageRevisionLifecycleDraft && pr.lifecycle != porchapi.PackageRevisionLifecycleProposed { + klog.Infof("PushDraftPackageRevision: repo %+v: PR %+v is no longer Draft/Proposed (lifecycle=%s), skipping (publish handles its own push)", repoKey, prKey, pr.lifecycle) + return + } + + if !prNeedsPushToGit(pr) { + klog.Infof("PushDraftPackageRevision: repo %+v: PR %+v already up to date in git, skipping", repoKey, prKey) + return + } + + resources := pr.resources + if len(resources) == 0 { + prResources, err := pr.GetResources(ctx) + if err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: failed to load resources for %+v: %v", repoKey, prKey, err) + return + } + if prResources != nil { + resources = prResources.Spec.Resources + } + } + + updatedBeforePush := pr.updated + + gitPRDraft, existingGitPR, err := GetOrCreateGitDraft(ctx, pr.repo.externalRepo, pr) + if err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: GetOrCreateGitDraft failed for %+v: %v", repoKey, prKey, err) + return + } + + if err = gitPRDraft.UpdateResources(ctx, &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: resources, + }, + }, commitTaskForPush(pr, existingGitPR != nil)); err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: UpdateResources failed for %+v: %v", repoKey, prKey, err) + return + } + + if err = gitPRDraft.UpdateLifecycle(ctx, pr.lifecycle); err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: UpdateLifecycle failed for %+v: %v", repoKey, prKey, err) + return + } + + pushedGitPR, err := pr.repo.externalRepo.ClosePackageRevisionDraft(ctx, gitPRDraft, prKey.Revision) + if err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: ClosePackageRevisionDraft failed for %+v: %v", repoKey, prKey, err) + return + } + + _, pushedLock, err := pushedGitPR.GetLock(ctx) + if err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: GetLock failed for %+v: %v", repoKey, prKey, err) + return + } + + recorded, err := pkgRevSetLastPushedInDB(ctx, prKey, pushedLock, updatedBeforePush) + if err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: failed to record push markers for %+v: %v", repoKey, prKey, err) + return + } + + if !recorded { + klog.Warningf("PushDraftPackageRevision: repo %+v: PR %+v was modified or published during push (updated changed from %v), not recording push markers — next sync will retry with fresh data", + repoKey, prKey, updatedBeforePush) + return + } + + commit := extPRCommitFromLocator(pushedLock) + klog.Infof("PushDraftPackageRevision: repo %+v: successfully pushed %+v to git at commit %q", repoKey, prKey, commit) +} + +func GetOrCreateGitDraft(ctx context.Context, repo repository.Repository, pr repository.PackageRevision) (draft repository.PackageRevisionDraft, updatedGitPR repository.PackageRevision, err error) { + prName := repository.ComposePkgRevObjName(pr.Key()) + klog.InfoS("[DBCache] Getting or creating Git draft for PackageRevision", + pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) + defer func() { + klog.V(3).InfoS("[DBCache] Get or create Git draft completed for PackageRevision", + pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) + }() + + existingPRs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Key: repository.PackageRevisionKey{ + PkgKey: pr.Key().PkgKey, + WorkspaceName: pr.Key().WorkspaceName, + }, + }) + + if err == nil && len(existingPRs) > 0 { + gitDraft, err := repo.UpdatePackageRevision(ctx, existingPRs[0]) + if err != nil { + return nil, nil, pkgerrors.Wrapf(err, "failed to update existing git branch for %+v", pr.Key()) + } + return gitDraft, existingPRs[0], nil + } + + apiPr, err := pr.GetPackageRevision(ctx) + if err != nil { + return nil, nil, pkgerrors.Wrapf(err, "failed to get API representation for %+v", pr.Key()) + } + + gitDraft, err := repo.CreatePackageRevisionDraft(ctx, apiPr) + if err != nil { + return nil, nil, pkgerrors.Wrapf(err, "failed to create git draft for %+v", pr.Key()) + } + + return gitDraft, nil, nil +} diff --git a/pkg/engine/pushpr_test.go b/pkg/cache/dbcache/dbpushtogit_test.go similarity index 86% rename from pkg/engine/pushpr_test.go rename to pkg/cache/dbcache/dbpushtogit_test.go index fa7128be8..91ac1363b 100644 --- a/pkg/engine/pushpr_test.go +++ b/pkg/cache/dbcache/dbpushtogit_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package engine +package dbcache import ( "context" @@ -145,7 +145,7 @@ func TestPushPublishedPackageRevision_PushDraftsDisabled(t *testing.T) { tt.setupMocks(mockRepo, mockPR, mockPRD) - _, err := PushPackageRevision(ctx, mockRepo, mockPR, false, nil) + _, err := PushPublishedPackageRevision(ctx, mockRepo, mockPR, false, false) if tt.expectError { assert.NotNil(t, err) } else { @@ -159,34 +159,27 @@ func TestPushPublishedPackageRevision_PushDraftsEnabled(t *testing.T) { ctx := context.TODO() tests := []struct { - name string - setupMocks func(*mockrepo.MockRepository, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevisionDraft) - gitPR bool - expectError bool + name string + setupMocks func(*mockrepo.MockRepository, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevisionDraft) + existingGitBranch bool + expectError bool }{ { - name: "Update existing PR", - gitPR: true, - setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { - mockPR.EXPECT().Lifecycle(mock.Anything).Return(porchapi.PackageRevisionLifecyclePublished).Once() - mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() - mockPR.EXPECT().GetResources(mock.Anything).Return(&porchapi.PackageRevisionResources{}, nil).Once() - mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(mockPRD, nil).Once() - mockPRD.EXPECT().UpdateLifecycle(mock.Anything, porchapi.PackageRevisionLifecyclePublished).Return(nil).Once() - mockRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, mockPRD, mock.Anything).Return(mockPR, nil).Once() - mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, nil).Once() - }, - expectError: false, - }, - { - name: "Existing PR found via list", - gitPR: false, + name: "Update existing PR", + existingGitBranch: true, setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { mockPR.EXPECT().Lifecycle(mock.Anything).Return(porchapi.PackageRevisionLifecyclePublished).Once() - mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() + mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{ + Spec: porchapi.PackageRevisionSpec{ + Tasks: []porchapi.Task{{Type: porchapi.TaskTypeEdit}}, + }, + }, nil).Once() mockPR.EXPECT().GetResources(mock.Anything).Return(&porchapi.PackageRevisionResources{}, nil).Once() mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{mockGitPR}, nil).Once() mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(mockPRD, nil).Once() + mockPRD.EXPECT().UpdateResources(mock.Anything, mock.Anything, mock.MatchedBy(func(task *porchapi.Task) bool { + return task != nil && task.Type == porchapi.TaskTypePush + })).Return(nil).Once() mockPRD.EXPECT().UpdateLifecycle(mock.Anything, porchapi.PackageRevisionLifecyclePublished).Return(nil).Once() mockRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, mockPRD, mock.Anything).Return(mockPR, nil).Once() mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, nil).Once() @@ -194,36 +187,41 @@ func TestPushPublishedPackageRevision_PushDraftsEnabled(t *testing.T) { expectError: false, }, { - name: "UpdatePackageRevision fails when gitPR provided", - gitPR: true, + name: "UpdatePackageRevision fails when existing git branch found", + existingGitBranch: true, setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { mockPR.EXPECT().Lifecycle(mock.Anything).Return(porchapi.PackageRevisionLifecyclePublished).Once() mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() mockPR.EXPECT().GetResources(mock.Anything).Return(&porchapi.PackageRevisionResources{}, nil).Once() + mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{mockGitPR}, nil).Once() mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(nil, assert.AnError).Once() }, expectError: true, }, { - name: "UpdatePackageRevision fails when gitPR found via list", - gitPR: false, + name: "ListPackageRevisions fails and falls back to creating package revision draft", + existingGitBranch: true, setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { mockPR.EXPECT().Lifecycle(mock.Anything).Return(porchapi.PackageRevisionLifecyclePublished).Once() - mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() + mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{Spec: porchapi.PackageRevisionSpec{Tasks: []porchapi.Task{{Type: porchapi.TaskTypePush}}}}, nil).Once() mockPR.EXPECT().GetResources(mock.Anything).Return(&porchapi.PackageRevisionResources{}, nil).Once() - mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{mockGitPR}, nil).Once() - mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(nil, assert.AnError).Once() + mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return(nil, assert.AnError).Once() + mockRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(mockPRD, nil).Once() + mockPRD.EXPECT().UpdateResources(mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + mockPRD.EXPECT().UpdateLifecycle(mock.Anything, porchapi.PackageRevisionLifecyclePublished).Return(nil).Once() + mockRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, mockPRD, mock.Anything).Return(mockPR, nil).Once() + mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, nil).Once() }, - expectError: true, + expectError: false, }, { - name: "ListPackageRevisions fails and falls back to creating package revision draft", - gitPR: false, + name: "ListPackageRevisions returns empty and falls back to creating package revision draft", + existingGitBranch: true, setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { mockPR.EXPECT().Lifecycle(mock.Anything).Return(porchapi.PackageRevisionLifecyclePublished).Once() - mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{Spec: porchapi.PackageRevisionSpec{Tasks: []porchapi.Task{{Type: porchapi.TaskTypePush}}}}, nil).Once() + mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() mockPR.EXPECT().GetResources(mock.Anything).Return(&porchapi.PackageRevisionResources{}, nil).Once() - mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return(nil, assert.AnError).Once() + mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{}, nil).Once() mockRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(mockPRD, nil).Once() mockPRD.EXPECT().UpdateResources(mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() mockPRD.EXPECT().UpdateLifecycle(mock.Anything, porchapi.PackageRevisionLifecyclePublished).Return(nil).Once() @@ -233,13 +231,12 @@ func TestPushPublishedPackageRevision_PushDraftsEnabled(t *testing.T) { expectError: false, }, { - name: "ListPackageRevisions returns empty and falls back to creating package revision draft", - gitPR: false, + name: "Creates new draft when no existing git branch", + existingGitBranch: false, setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { mockPR.EXPECT().Lifecycle(mock.Anything).Return(porchapi.PackageRevisionLifecyclePublished).Once() mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() mockPR.EXPECT().GetResources(mock.Anything).Return(&porchapi.PackageRevisionResources{}, nil).Once() - mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{}, nil).Once() mockRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(mockPRD, nil).Once() mockPRD.EXPECT().UpdateResources(mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() mockPRD.EXPECT().UpdateLifecycle(mock.Anything, porchapi.PackageRevisionLifecyclePublished).Return(nil).Once() @@ -260,14 +257,9 @@ func TestPushPublishedPackageRevision_PushDraftsEnabled(t *testing.T) { mockRepo.EXPECT().Key().Return(repository.RepositoryKey{}).Maybe() mockPR.EXPECT().Key().Return(repository.PackageRevisionKey{}).Maybe() - var gitPR repository.PackageRevision - if tt.gitPR { - gitPR = mockGitPR - } - tt.setupMocks(mockRepo, mockPR, mockGitPR, mockPRD) - _, err := PushPackageRevision(ctx, mockRepo, mockPR, true, gitPR) + _, err := PushPublishedPackageRevision(ctx, mockRepo, mockPR, true, tt.existingGitBranch) if tt.expectError { assert.NotNil(t, err) } else { @@ -283,35 +275,15 @@ func TestGetOrCreateGitDraft(t *testing.T) { tests := []struct { name string setupMocks func(*mockrepo.MockRepository, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevisionDraft) - gitPR bool expectError bool expectUpdatedGitPR bool }{ { - name: "UpdatePackageRevision succeeds", - setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { - mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(mockPRD, nil).Once() - }, - gitPR: true, - expectError: false, - expectUpdatedGitPR: true, - }, - { - name: "UpdatePackageRevision fails", - setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { - mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(nil, assert.AnError).Once() - }, - gitPR: true, - expectError: true, - expectUpdatedGitPR: false, - }, - { - name: "Existing PRs found", + name: "UpdatePackageRevision succeeds when existing PRs found", setupMocks: func(mockRepo *mockrepo.MockRepository, mockPR *mockrepo.MockPackageRevision, mockGitPR *mockrepo.MockPackageRevision, mockPRD *mockrepo.MockPackageRevisionDraft) { mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{mockGitPR}, nil).Once() mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(mockPRD, nil).Once() }, - gitPR: false, expectError: false, expectUpdatedGitPR: true, }, @@ -321,7 +293,6 @@ func TestGetOrCreateGitDraft(t *testing.T) { mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{mockGitPR}, nil).Once() mockRepo.EXPECT().UpdatePackageRevision(mock.Anything, mockGitPR).Return(nil, assert.AnError).Once() }, - gitPR: false, expectError: true, expectUpdatedGitPR: false, }, @@ -332,7 +303,6 @@ func TestGetOrCreateGitDraft(t *testing.T) { mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() mockRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(mockPRD, nil).Once() }, - gitPR: false, expectError: false, expectUpdatedGitPR: false, }, @@ -343,7 +313,6 @@ func TestGetOrCreateGitDraft(t *testing.T) { mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() mockRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(mockPRD, nil).Once() }, - gitPR: false, expectError: false, expectUpdatedGitPR: false, }, @@ -353,7 +322,6 @@ func TestGetOrCreateGitDraft(t *testing.T) { mockRepo.EXPECT().ListPackageRevisions(mock.Anything, mock.Anything).Return([]repository.PackageRevision{}, nil).Once() mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(nil, assert.AnError).Once() }, - gitPR: false, expectError: true, expectUpdatedGitPR: false, }, @@ -364,7 +332,6 @@ func TestGetOrCreateGitDraft(t *testing.T) { mockPR.EXPECT().GetPackageRevision(mock.Anything).Return(&porchapi.PackageRevision{}, nil).Once() mockRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(nil, assert.AnError).Once() }, - gitPR: false, expectError: true, expectUpdatedGitPR: false, }, @@ -379,14 +346,9 @@ func TestGetOrCreateGitDraft(t *testing.T) { mockPR.EXPECT().Key().Return(repository.PackageRevisionKey{}).Maybe() - var gitPR repository.PackageRevision - if tt.gitPR { - gitPR = mockGitPR - } - tt.setupMocks(mockRepo, mockPR, mockGitPR, mockPRD) - draft, updatedGitPR, err := GetOrCreateGitDraft(ctx, mockRepo, mockPR, gitPR) + draft, updatedGitPR, err := GetOrCreateGitDraft(ctx, mockRepo, mockPR) if tt.expectError { assert.NotNil(t, err) diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 147402859..017f33c02 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -19,7 +19,6 @@ import ( "database/sql" "fmt" "slices" - "sync" "time" kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" @@ -27,7 +26,6 @@ import ( configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" "github.com/kptdev/porch/internal/telemetry" cachetypes "github.com/kptdev/porch/pkg/cache/types" - "github.com/kptdev/porch/pkg/engine" "github.com/kptdev/porch/pkg/externalrepo" externalrepotypes "github.com/kptdev/porch/pkg/externalrepo/types" "github.com/kptdev/porch/pkg/repository" @@ -52,44 +50,7 @@ type dbRepository struct { updatedBy string deployment bool repoPRChangeNotifier cachetypes.RepoPRChangeNotifier - - pushDraftsToGit bool - gitPRCacheMutex sync.RWMutex - gitPRCache map[string]repository.PackageRevision -} - -func (r *dbRepository) gitPRCacheKey(pkgKey repository.PackageKey, workspaceName string) string { - return fmt.Sprintf("%s/%s/%s", pkgKey.Package, pkgKey.Path, workspaceName) -} - -func (r *dbRepository) getCachedGitPR(pkgKey repository.PackageKey, workspaceName string) repository.PackageRevision { - r.gitPRCacheMutex.RLock() - defer r.gitPRCacheMutex.RUnlock() - - cacheKey := r.gitPRCacheKey(pkgKey, workspaceName) - return r.gitPRCache[cacheKey] -} - -func (r *dbRepository) setCachedGitPR(pkgKey repository.PackageKey, workspaceName string, gitPR repository.PackageRevision) { - if gitPR == nil { - return - } - - r.gitPRCacheMutex.Lock() - defer r.gitPRCacheMutex.Unlock() - - cacheKey := r.gitPRCacheKey(pkgKey, workspaceName) - r.gitPRCache[cacheKey] = gitPR - klog.V(5).Infof("cached gitPR for %s", cacheKey) -} - -func (r *dbRepository) deleteCachedGitPR(pkgKey repository.PackageKey, workspaceName string) { - r.gitPRCacheMutex.Lock() - defer r.gitPRCacheMutex.Unlock() - - cacheKey := r.gitPRCacheKey(pkgKey, workspaceName) - delete(r.gitPRCache, cacheKey) - klog.V(5).Infof("deleted cached gitPR for %s", cacheKey) + pushDraftsToGit bool } func (r *dbRepository) KubeObjectName() string { @@ -114,10 +75,6 @@ func (r *dbRepository) OpenRepository(ctx context.Context, externalRepoOptions e klog.V(5).Infof("dbRepository:OpenRepository: opening repository %+v", r.Key()) - if r.pushDraftsToGit { - r.gitPRCache = make(map[string]repository.PackageRevision) - } - externalRepo, err := externalrepo.CreateRepositoryImpl(ctx, r.spec, externalRepoOptions) if err != nil { klog.Warningf("dbRepository:OpenRepository: repo %+v connectivity check failed with error %q", r.Key(), err) @@ -255,6 +212,14 @@ func (r *dbRepository) CreatePackageRevisionDraft(ctx context.Context, newPR *po deployment: r.deployment, } + if len(newPR.Spec.Tasks) > 0 { + if porchapi.IsValidFirstTaskType(newPR.Spec.Tasks[0].Type) { + dbPkgRev.tasks = []porchapi.Task{newPR.Spec.Tasks[0]} + } else { + klog.Warningf("dbRepository:CreatePackageRevisionDraft: invalid first task type %q for %+v on repo %+v", newPR.Spec.Tasks[0].Type, newPR, r.Key()) + } + } + dbPkgRev.meta.CreationTimestamp = metav1.Time{Time: time.Now()} dbPkgRev.extPRID = kptfilev1.Locator{ @@ -263,25 +228,10 @@ func (r *dbRepository) CreatePackageRevisionDraft(ctx context.Context, newPR *po Repo: dbPkgRev.repo.spec.Spec.Git.Repo, Directory: dbPkgRev.Key().PKey().ToPkgPathname(), Ref: "drafts/" + dbPkgRev.Key().PKey().ToPkgPathname() + "/" + dbPkgRev.Key().WorkspaceName, - Commit: "not-pushed", + Commit: unpushedGitCommit, }, } - if r.pushDraftsToGit { - prName := repository.ComposePkgRevObjName(dbPkgRev.Key()) - klog.InfoS("[DB Cache] Creating draft in Git for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - defer func() { - klog.V(3).InfoS("[DB Cache] Draft created in Git for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - }() - gitPRDraft, err := r.externalRepo.CreatePackageRevisionDraft(ctx, newPR) - if err != nil { - return nil, pkgerrors.Wrapf(err, "failed to create git draft for %+v, not saving to DB", dbPkgRev.Key()) - } - dbPkgRev.gitPRDraft = gitPRDraft - } - if prDraft, err := r.savePackageRevisionDraft(ctx, dbPkgRev, 0); err == nil { return repository.PackageRevisionDraft(prDraft), nil } else { @@ -337,6 +287,9 @@ func (r *dbRepository) DeletePackageRevision(ctx context.Context, pr2Delete repo Package: pr2Delete.Key().PKey().Package, } + lockPkgKey(pk) + defer unlockPkgKey(pk) + foundPkg, err := pkgReadFromDB(ctx, pk) if err != nil { return err @@ -347,10 +300,6 @@ func (r *dbRepository) DeletePackageRevision(ctx context.Context, pr2Delete repo return err } - if r.pushDraftsToGit { - r.deleteCachedGitPR(pr2Delete.Key().PkgKey, pr2Delete.Key().WorkspaceName) - } - foundPRs, err := pkgRevReadPRsFromDB(ctx, foundPkg.Key()) if err != nil { return err @@ -390,32 +339,22 @@ func (r *dbRepository) UpdatePackageRevision(ctx context.Context, updatePR repos updatePkgRev.repo = r } + lockPkgKey(updatePkgRev.Key().PKey()) + defer unlockPkgKey(updatePkgRev.Key().PKey()) + if err := updatePkgRev.UpdatePackageRevision(ctx); err != nil { return nil, err } + if existing, err := pkgRevReadFromDB(ctx, updatePkgRev.Key(), false); err == nil { + preservePushMarkersIfUnset(updatePkgRev, existing) + } else if err != sql.ErrNoRows { + return nil, err + } + updatePkgRev.updated = time.Now() updatePkgRev.updatedBy = getCurrentUser() - if r.pushDraftsToGit && updatePkgRev.gitPRDraft == nil { - klog.InfoS("[DB Cache] Getting or creating Git draft for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", repository.ComposePkgRevObjName(updatePkgRev.Key()))...) - defer func() { - klog.V(3).InfoS("[DB Cache] Git draft get or create completed for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", repository.ComposePkgRevObjName(updatePkgRev.Key()))...) - }() - gitPRToUse := r.getCachedGitPR(updatePkgRev.Key().PkgKey, updatePkgRev.Key().WorkspaceName) - - gitPRDraft, gitPR, err := engine.GetOrCreateGitDraft(ctx, r.externalRepo, updatePkgRev, gitPRToUse) - if err != nil { - return nil, pkgerrors.Wrapf(err, "failed to get or create git draft for %+v", updatePkgRev.Key()) - } - updatePkgRev.gitPRDraft = gitPRDraft - if gitPR != nil { - updatePkgRev.gitPR = gitPR - } - } - return updatePkgRev, nil } @@ -466,22 +405,6 @@ func (r *dbRepository) ClosePackageRevisionDraft(ctx context.Context, prd reposi dbPrd := prd.(*dbPackageRevision) - if r.pushDraftsToGit && dbPrd.gitPRDraft != nil { - klog.InfoS("[DB Cache] Closing Git draft and pushing to Git for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", repository.ComposePkgRevObjName(dbPrd.Key()))...) - defer func() { - klog.V(3).InfoS("[DB Cache] Git draft closed and pushed for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", repository.ComposePkgRevObjName(dbPrd.Key()))...) - }() - gitPR, err := r.externalRepo.ClosePackageRevisionDraft(ctx, dbPrd.gitPRDraft, version) - if err != nil { - return nil, pkgerrors.Wrapf(err, "failed to close git draft for %+v, not saving to DB", dbPrd.Key()) - } - dbPrd.gitPR = gitPR - dbPrd.gitPRDraft = nil - r.setCachedGitPR(dbPrd.Key().PkgKey, dbPrd.Key().WorkspaceName, gitPR) - } - dbPrd.resourcesSizeBytes = 0 for _, fileString := range dbPrd.resources { dbPrd.resourcesSizeBytes += int64(len(fileString)) @@ -494,16 +417,6 @@ func (r *dbRepository) ClosePackageRevisionDraft(ctx context.Context, prd reposi telemetry.RecordPackageRevisionResourcesSize(ctx, pr.Key(), pr.resourcesSizeBytes) - if r.pushDraftsToGit && pr.gitPRDraft != nil && r.externalRepo != nil { - gitPR, err := r.externalRepo.ClosePackageRevisionDraft(ctx, pr.gitPRDraft, 0) - if err != nil { - klog.Warningf("failed to close git draft for %+v: %v", pr.Key(), err) - } else { - pr.gitPR = gitPR - pr.gitPRDraft = nil - } - } - return repository.PackageRevision(pr), nil } @@ -513,6 +426,9 @@ func (r *dbRepository) savePackageRevisionDraft(ctx context.Context, prd reposit d := prd.(*dbPackageRevision) + lockRepoKey(r.repoKey) + defer unlockRepoKey(r.repoKey) + return r.savePackageRevision(ctx, d, d.resourcesDirty) } @@ -520,6 +436,9 @@ func (r *dbRepository) savePackageRevision(ctx context.Context, d *dbPackageRevi _, span := tracer.Start(ctx, "dbRepository::savePackageRevision", trace.WithAttributes()) defer span.End() + lockPkgKey(d.Key().PKey()) + defer unlockPkgKey(d.Key().PKey()) + dbPkg, err := pkgReadFromDB(ctx, d.Key().PKey()) if err != nil { if err != sql.ErrNoRows { @@ -556,9 +475,15 @@ func (r *dbRepository) Refresh(ctx context.Context) error { _, span := tracer.Start(ctx, "dbRepository::Refresh", trace.WithAttributes()) defer span.End() + lockRepoKey(r.Key()) + defer unlockRepoKey(r.Key()) + + r.repositorySync.mutex.Lock() if err := r.externalRepo.Refresh(ctx); err != nil { + r.repositorySync.mutex.Unlock() return err } + r.repositorySync.mutex.Unlock() if err := r.repositorySync.SyncOnce(ctx); err != nil { klog.Warningf("sync returned error %q", err) diff --git a/pkg/cache/dbcache/dbrepository_test.go b/pkg/cache/dbcache/dbrepository_test.go index 5d833521d..59f40f225 100644 --- a/pkg/cache/dbcache/dbrepository_test.go +++ b/pkg/cache/dbcache/dbrepository_test.go @@ -236,9 +236,6 @@ func (t *DbTestSuite) newPushDraftsToGitTestRepo(namespace, name string, pushDra testRepo.externalRepo = extRepo testRepo.pushDraftsToGit = pushDraftsToGit - if pushDraftsToGit { - testRepo.gitPRCache = make(map[string]repository.PackageRevision) - } return testRepo, extRepo } @@ -254,11 +251,6 @@ func (t *DbTestSuite) TestDBRepositoryDeleteDraftWithPushDraftsToGit() { testRepo, extRepo := t.newPushDraftsToGitTestRepo(namespace, repoName, true) - gitDraft := mockrepo.NewMockPackageRevisionDraft(t.T()) - gitPR := mockrepo.NewMockPackageRevision(t.T()) - extRepo.EXPECT().CreatePackageRevisionDraft(mock.Anything, mock.Anything).Return(gitDraft, nil).Once() - extRepo.EXPECT().ClosePackageRevisionDraft(mock.Anything, gitDraft, 0).Return(gitPR, nil).Once() - newPRDef := porchapi.PackageRevision{ Spec: porchapi.PackageRevisionSpec{ RepositoryName: repoName, @@ -274,10 +266,9 @@ func (t *DbTestSuite) TestDBRepositoryDeleteDraftWithPushDraftsToGit() { dbPR, err := testRepo.ClosePackageRevisionDraft(ctx, prDraft, 0) t.Require().NoError(err) t.Require().Equal(porchapi.PackageRevisionLifecycleDraft, dbPR.Lifecycle(ctx)) - t.Require().Len(testRepo.gitPRCache, 1) - // The draft is unpublished, so this is the assertion that would fail if the external - // delete were still gated on the lifecycle being published. + // Drafts are stored in the database; git cleanup on delete is still required when pushDraftsToGit is set. + // This would fail if the external delete were still gated on the lifecycle being published. extRepo.EXPECT().DeletePackageRevision(mock.Anything, dbPR).Return(nil).Once() err = testRepo.DeletePackageRevision(ctx, dbPR) @@ -286,7 +277,6 @@ func (t *DbTestSuite) TestDBRepositoryDeleteDraftWithPushDraftsToGit() { prList, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) t.Require().NoError(err) t.Empty(prList, "package revision should be gone from the database") - t.Empty(testRepo.gitPRCache, "cached git package revision should be evicted on delete") t.deleteTestRepo(testRepo.Key()) } diff --git a/pkg/cache/dbcache/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index ec8ee1827..44a611ed9 100644 --- a/pkg/cache/dbcache/dbreposync.go +++ b/pkg/cache/dbcache/dbreposync.go @@ -16,6 +16,7 @@ package dbcache import ( "context" + "database/sql" "fmt" "strings" stdSync "sync" @@ -98,7 +99,7 @@ func (s *repositorySync) sync(ctx context.Context) (repositorySyncStats, error) inCachedOnly, inBoth, inExternalOnly := s.comparePRMaps(ctx, cachedPrMap, externalPrMap) klog.Infof("repositorySync %+v: found %d cached only, %d in both, %d external only", s.repo.Key(), len(inCachedOnly), len(inBoth), len(inExternalOnly)) - if err = s.deletePRsOnlyInCache(ctx, cachedPrMap, inCachedOnly); err != nil { + if err = s.handleInCachedOnly(ctx, cachedPrMap, inCachedOnly); err != nil { return repositorySyncStats{}, err } @@ -106,6 +107,10 @@ func (s *repositorySync) sync(ctx context.Context) (repositorySyncStats, error) return repositorySyncStats{}, err } + if s.repo.pushDraftsToGit { + s.handleInBoth(ctx, cachedPrMap, inBoth) + } + return repositorySyncStats{ cachedOnly: len(inCachedOnly), externalOnly: len(inExternalOnly), @@ -198,25 +203,7 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map } // Guard against nil return from GetResources (interface contract allows it). - var resources map[string]string - var resourcesSize int64 - if extPRResources == nil || extPRResources.Spec.Resources == nil { - resources = make(map[string]string) - resourcesSize = 0 - } else { - // Filter out files with invalid UTF-8 or NUL bytes to avoid PostgreSQL TEXT errors. - // Both resource_key and resource_value are TEXT columns, so both must be validated. - resources = make(map[string]string, len(extPRResources.Spec.Resources)) - for key, val := range extPRResources.Spec.Resources { - if !utf8.ValidString(key) || strings.Contains(key, "\x00") || - !utf8.ValidString(val) || strings.Contains(val, "\x00") { - klog.Warningf("repositorySync %+v: skipping file %q in PR %+v (not compatible with PostgreSQL TEXT)", s.repo.Key(), key, extPRKey) - continue - } - resources[key] = val - resourcesSize += int64(len(val)) - } - } + resources, resourcesSize := s.sanitizeResources(extPRKey, extPRResources) if extAPIPR.CreationTimestamp.Time.IsZero() { extAPIPR.CreationTimestamp.Time = time.Now() @@ -224,6 +211,12 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map _, extPRUpstreamLock, _ := extPR.GetLock(ctx) + if extPRKey.Revision == 0 && porchapi.LifecycleIsPublished(extAPIPR.Spec.Lifecycle) { + klog.Warningf("repositorySync %+v: skipping external package revision %+v with invalid combination (revision=0, lifecycle=%s)", + s.repo.Key(), extPRKey, extAPIPR.Spec.Lifecycle) + continue + } + dbPR := dbPackageRevision{ repo: s.repo, pkgRevKey: extPRKey, @@ -250,26 +243,128 @@ func (s *repositorySync) cacheExternalPRs(ctx context.Context, externalPrMap map return nil } -func (s *repositorySync) deletePRsOnlyInCache(ctx context.Context, cachedPrMap map[repository.PackageRevisionKey]repository.PackageRevision, inCachedOnly []repository.PackageRevisionKey) error { +// sanitizeResources copies an external package revision's resources, dropping any files whose key or +// value contains invalid UTF-8 or NUL bytes (which PostgreSQL TEXT columns cannot store). +func (s *repositorySync) sanitizeResources(prKey repository.PackageRevisionKey, extPRResources *porchapi.PackageRevisionResources) (map[string]string, int64) { + var resources map[string]string + var resourcesSize int64 + if extPRResources == nil || extPRResources.Spec.Resources == nil { + resources = make(map[string]string) + resourcesSize = 0 + } else { + // Filter out files with invalid UTF-8 or NUL bytes to avoid PostgreSQL TEXT errors. + // Both resource_key and resource_value are TEXT columns, so both must be validated. + resources = make(map[string]string, len(extPRResources.Spec.Resources)) + for key, val := range extPRResources.Spec.Resources { + if !utf8.ValidString(key) || strings.Contains(key, "\x00") || + !utf8.ValidString(val) || strings.Contains(val, "\x00") { + klog.Warningf("repositorySync %+v: skipping file %q in PR %+v (not compatible with PostgreSQL TEXT)", s.repo.Key(), key, prKey) + continue + } + resources[key] = val + resourcesSize += int64(len(val)) + } + } + return resources, resourcesSize +} + +func (s *repositorySync) handleInCachedOnly(ctx context.Context, cachedPrMap map[repository.PackageRevisionKey]repository.PackageRevision, inCachedOnly []repository.PackageRevisionKey) error { + var prsToPush []*dbPackageRevision + for _, dbPRKey := range inCachedOnly { dbPR := cachedPrMap[dbPRKey] - pkgList, err := s.repo.ListPackages(ctx, repository.ListPackageFilter{Key: dbPR.Key().PKey()}) - if err != nil { - return err + if dbPkgRev, ok := dbPR.(*dbPackageRevision); ok && + (dbPkgRev.lifecycle == porchapi.PackageRevisionLifecycleDraft || dbPkgRev.lifecycle == porchapi.PackageRevisionLifecycleProposed) && + !hasBeenPushedToGit(dbPkgRev) { + if s.repo.pushDraftsToGit { + klog.Infof("repositorySync %+v: cached-only %s PR %+v has not been pushed to git yet, queuing for push instead of deleting", s.repo.Key(), dbPkgRev.lifecycle, dbPRKey) + prsToPush = append(prsToPush, dbPkgRev) + } else { + klog.Infof("repositorySync %+v: skipping deletion of cached %s PR %+v because it has not been pushed to git yet", s.repo.Key(), dbPkgRev.lifecycle, dbPRKey) + } + continue } - if len(pkgList) != 1 { - err := fmt.Errorf("deletePRsOnlyInCache: reading package %+v should return 1 package, it returned %d packages", dbPR.Key().PKey(), len(pkgList)) - klog.Warning(err.Error()) + if err := s.deleteCachedOnlyPR(ctx, dbPRKey, dbPR); err != nil { return err } + } - dbPkg := pkgList[0].(*dbPackage) - if err = dbPkg.DeletePackageRevision(ctx, dbPR, false); err != nil { - klog.Errorf("repositorySync %+v: failed to delete cached PR %+v not in external repo", s.repo.Key(), dbPRKey) - return err + for _, pr := range prsToPush { + s.enqueuePush(ctx, pr) + } + + return nil +} + +func (s *repositorySync) deleteCachedOnlyPR(ctx context.Context, dbPRKey repository.PackageRevisionKey, snapshot repository.PackageRevision) error { + pkgKey := dbPRKey.PKey() + lockPkgKey(pkgKey) + defer unlockPkgKey(pkgKey) + + freshPR, err := pkgRevReadFromDB(ctx, dbPRKey, false) + if err != nil { + if err == sql.ErrNoRows { + klog.Infof("repositorySync %+v: handleInCachedOnly: PR %+v already removed from the database, skipping deletion", s.repo.Key(), dbPRKey) + return nil } + return err } + + if snap, ok := snapshot.(*dbPackageRevision); ok && !freshPR.updated.Equal(snap.updated) { + klog.Infof("repositorySync %+v: handleInCachedOnly: PR %+v changed since the cached list was taken (updated %v -> %v), skipping deletion", s.repo.Key(), dbPRKey, snap.updated, freshPR.updated) + return nil + } + + if (freshPR.lifecycle == porchapi.PackageRevisionLifecycleDraft || freshPR.lifecycle == porchapi.PackageRevisionLifecycleProposed) && + !hasBeenPushedToGit(freshPR) { + klog.Infof("repositorySync %+v: handleInCachedOnly: PR %+v is now an unpushed %s revision, skipping deletion", s.repo.Key(), dbPRKey, freshPR.lifecycle) + return nil + } + + pkgList, err := s.repo.ListPackages(ctx, repository.ListPackageFilter{Key: pkgKey}) + if err != nil { + return err + } + + if len(pkgList) != 1 { + err := fmt.Errorf("handleInCachedOnly: reading package %+v should return 1 package, it returned %d packages", pkgKey, len(pkgList)) + klog.Warning(err.Error()) + return err + } + + dbPkg := pkgList[0].(*dbPackage) + if err = dbPkg.DeletePackageRevision(ctx, freshPR, false); err != nil { + klog.Errorf("repositorySync %+v: failed to delete cached PR %+v not in external repo", s.repo.Key(), dbPRKey) + return err + } + return nil } + +func (s *repositorySync) handleInBoth(ctx context.Context, cachedPrMap map[repository.PackageRevisionKey]repository.PackageRevision, inBoth []repository.PackageRevisionKey) { + ctx, span := tracer.Start(ctx, "Repository::handleInBoth", trace.WithAttributes()) + defer span.End() + + for _, prKey := range inBoth { + cachedPR, ok := cachedPrMap[prKey].(*dbPackageRevision) + if !ok { + continue + } + + if cachedPR.lifecycle != porchapi.PackageRevisionLifecycleDraft && cachedPR.lifecycle != porchapi.PackageRevisionLifecycleProposed { + continue + } + + if dbContentChangedSincePush(cachedPR) { + klog.Infof("repositorySync %+v: reconcile %+v: DB changed since last push, pushing to git", s.repo.Key(), prKey) + s.enqueuePush(ctx, cachedPR) + } + } +} + +func (s *repositorySync) enqueuePush(ctx context.Context, pr *dbPackageRevision) { + pushCtx := context.WithoutCancel(ctx) + go PushDraftPackageRevision(pushCtx, s.repo.Key(), pr) +} diff --git a/pkg/cache/dbcache/dbreposync_test.go b/pkg/cache/dbcache/dbreposync_test.go index 7dbc6763e..69c2c0fd0 100644 --- a/pkg/cache/dbcache/dbreposync_test.go +++ b/pkg/cache/dbcache/dbreposync_test.go @@ -215,7 +215,7 @@ func (t *DbTestSuite) TestDBRepoSyncWithPushDraftsToGit_DraftInExternalKept() { t.Require().NoError(err) } -func (t *DbTestSuite) TestDBRepoSyncWithPushDraftsToGit_DraftOnlyInCacheRemoved() { +func (t *DbTestSuite) TestDBRepoSyncWithPushDraftsToGit_DraftOnlyInCacheQueuedForPush() { mockCache := mockcachetypes.NewMockCache(t.T()) cachetypes.CacheInstance = mockCache repoName := "push-drafts-removed-repo" @@ -260,17 +260,27 @@ func (t *DbTestSuite) TestDBRepoSyncWithPushDraftsToGit_DraftOnlyInCacheRemoved( t.Require().NoError(err) t.Require().NotNil(dbPRDraft) - _, err = testRepo.ClosePackageRevisionDraft(ctx, dbPRDraft, 0) + closedPR, err := testRepo.ClosePackageRevisionDraft(ctx, dbPRDraft, 0) t.Require().NoError(err) + t.Require().NotNil(closedPR) + prKey := closedPR.Key() - // Do not add the draft to the external repo. Sync should treat it as "cached only" and remove it. + // Do not add the draft to the external repo. Sync should queue a git push instead of deleting it. // Explicitly trigger sync err = testRepo.repositorySync.SyncOnce(ctx) t.Require().NoError(err) + t.Eventually(func() bool { + freshPR, err := pkgRevReadFromDB(ctx, prKey, false) + if err != nil { + return false + } + return hasBeenPushedToGit(freshPR) + }, 5*time.Second, 50*time.Millisecond, "async PushDraftPackageRevision should record last_pushed_db_updated in DB") + prList, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) t.Require().NoError(err) - t.Equal(0, len(prList), "with pushDraftsToGit enabled, draft only in cache should be removed by sync") + t.Equal(1, len(prList), "with pushDraftsToGit enabled, draft only in cache should be pushed to git, not removed") err = testRepo.Close(ctx) t.Require().NoError(err) @@ -1111,3 +1121,518 @@ func (t *DbTestSuite) TestCacheExternalPRs_NilResources() { t.Require().NoError(err) t.Empty(cachedResources, "nil resources should result in empty cached resources") } + +// TestComparePRMaps verifies that comparePRMaps correctly partitions keys into +// left-only, both, and right-only sets. +func (t *DbTestSuite) TestComparePRMaps() { + testRepo := t.createTestRepo("compare-ns", "compare-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + repoKey := testRepo.Key() + + keyA := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repoKey, Package: "pkg-a"}, + Revision: 1, + WorkspaceName: "ws1", + } + keyB := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repoKey, Package: "pkg-b"}, + Revision: 1, + WorkspaceName: "ws1", + } + keyC := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repoKey, Package: "pkg-c"}, + Revision: 1, + WorkspaceName: "ws1", + } + + leftMap := map[repository.PackageRevisionKey]repository.PackageRevision{ + keyA: nil, // left only + keyB: nil, // in both + } + rightMap := map[repository.PackageRevisionKey]repository.PackageRevision{ + keyB: nil, // in both + keyC: nil, // right only + } + + leftOnly, both, rightOnly := s.comparePRMaps(t.Context(), leftMap, rightMap) + + t.Len(leftOnly, 1) + t.Equal(keyA, leftOnly[0]) + + t.Len(both, 1) + t.Equal(keyB, both[0]) + + t.Len(rightOnly, 1) + t.Equal(keyC, rightOnly[0]) +} + +// TestComparePRMaps_EmptyMaps verifies comparePRMaps handles empty inputs correctly. +func (t *DbTestSuite) TestComparePRMaps_EmptyMaps() { + testRepo := t.createTestRepo("compareempty-ns", "compareempty-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + + leftOnly, both, rightOnly := s.comparePRMaps( + t.Context(), + map[repository.PackageRevisionKey]repository.PackageRevision{}, + map[repository.PackageRevisionKey]repository.PackageRevision{}, + ) + + t.Empty(leftOnly) + t.Empty(both) + t.Empty(rightOnly) +} + +// TestComparePRMaps_AllInBoth verifies comparePRMaps when both maps are identical. +func (t *DbTestSuite) TestComparePRMaps_AllInBoth() { + testRepo := t.createTestRepo("compareboth-ns", "compareboth-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + repoKey := testRepo.Key() + + key1 := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repoKey, Package: "pkg-1"}, + Revision: 1, + WorkspaceName: "ws1", + } + key2 := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: repoKey, Package: "pkg-2"}, + Revision: 2, + WorkspaceName: "ws2", + } + + sharedMap := map[repository.PackageRevisionKey]repository.PackageRevision{ + key1: nil, + key2: nil, + } + + leftOnly, both, rightOnly := s.comparePRMaps(t.Context(), sharedMap, sharedMap) + + t.Empty(leftOnly) + t.Len(both, 2) + t.Empty(rightOnly) +} + +// TestSanitizeResources_NilInput verifies sanitizeResources returns empty map for nil input. +func (t *DbTestSuite) TestSanitizeResources_NilInput() { + testRepo := t.createTestRepo("san-nil-ns", "san-nil-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + prKey := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: testRepo.Key(), Package: "pkg"}, + Revision: 1, + WorkspaceName: "ws", + } + + result, _ := s.sanitizeResources(prKey, nil) + t.Empty(result, "nil resources should return empty map") +} + +// TestSanitizeResources_NilResourceMap verifies sanitizeResources handles nil Resources map. +func (t *DbTestSuite) TestSanitizeResources_NilResourceMap() { + testRepo := t.createTestRepo("san-nilmap-ns", "san-nilmap-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + prKey := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: testRepo.Key(), Package: "pkg"}, + Revision: 1, + WorkspaceName: "ws", + } + + result, _ := s.sanitizeResources(prKey, &porchapi.PackageRevisionResources{}) + t.Empty(result, "nil Resources map should return empty map") +} + +// TestSanitizeResources_FiltersInvalidContent verifies sanitizeResources drops +// files with invalid UTF-8 or NUL bytes in key or value. +func (t *DbTestSuite) TestSanitizeResources_FiltersInvalidContent() { + testRepo := t.createTestRepo("san-filter-ns", "san-filter-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + prKey := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: testRepo.Key(), Package: "pkg"}, + Revision: 1, + WorkspaceName: "ws", + } + + resources := &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "valid.yaml": "valid content", + "invalid\xff.yaml": "valid content", // invalid UTF-8 in key + "nul-in-value.yaml": "content\x00here", // NUL byte in value + "nul-in-key\x00": "valid content", // NUL byte in key + "binary.bin": "\x89PNG\r\n\x1a\n", // binary content + }, + }, + } + + result, _ := s.sanitizeResources(prKey, resources) + + t.Len(result, 1, "only the valid file should be retained") + t.Contains(result, "valid.yaml") + t.NotContains(result, "invalid\xff.yaml") + t.NotContains(result, "nul-in-value.yaml") + t.NotContains(result, "nul-in-key\x00") + t.NotContains(result, "binary.bin") +} + +// TestSanitizeResources_AllValid verifies sanitizeResources returns all files when all are valid. +func (t *DbTestSuite) TestSanitizeResources_AllValid() { + testRepo := t.createTestRepo("san-allvalid-ns", "san-allvalid-repo") + defer t.deleteTestRepo(testRepo.Key()) + + s := &repositorySync{repo: testRepo} + prKey := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{RepoKey: testRepo.Key(), Package: "pkg"}, + Revision: 1, + WorkspaceName: "ws", + } + + resources := &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": "apiVersion: kpt.dev/v1\nkind: Kptfile\n", + "config.yaml": "key: value\n", + "README.md": "# Hello World\n", + }, + }, + } + + result, _ := s.sanitizeResources(prKey, resources) + t.Len(result, 3, "all valid files should be retained") +} + +// TestCacheExternalPRs_SkipsRevision0Published verifies that a PR with revision=0 and +// Published lifecycle is skipped and not cached, since that combination is invalid. +func (t *DbTestSuite) TestCacheExternalPRs_SkipsRevision0Published() { + ctx := t.Context() + externalrepo.ExternalRepoInUnitTestMode = true + + testRepo := t.createTestRepo("r0pub-ns", "r0pub-repo") + defer t.deleteTestRepo(testRepo.Key()) + + mockCache := mockcachetypes.NewMockCache(t.T()) + cachetypes.CacheInstance = mockCache + mockCache.EXPECT().GetRepository(mock.Anything).Return(testRepo).Maybe() + + err := testRepo.OpenRepository(ctx, externalrepotypes.ExternalRepoOptions{}) + t.Require().NoError(err) + defer func() { + if err := testRepo.Close(ctx); err != nil { + t.T().Logf("Failed to close test repo: %v", err) + } + }() + + repoSync := &repositorySync{repo: testRepo} + + // revision=0 with Published lifecycle is an invalid combination that should be skipped + prKey := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{ + RepoKey: testRepo.Key(), + Package: "r0pub-pkg", + }, + Revision: 0, // invalid: revision=0 with Published lifecycle + WorkspaceName: "ws", + } + + prDef := &porchapi.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "r0pub-pr", + Namespace: "r0pub-ns", + CreationTimestamp: metav1.Now(), + }, + Spec: porchapi.PackageRevisionSpec{ + RepositoryName: "r0pub-repo", + PackageName: "r0pub-pkg", + WorkspaceName: "ws", + Lifecycle: porchapi.PackageRevisionLifecyclePublished, + }, + } + + resources := &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": "apiVersion: kpt.dev/v1\nkind: Kptfile\n", + }, + }, + } + + fakeExtPR := &fake.FakePackageRevision{ + PrKey: prKey, + PackageRevision: prDef, + PackageLifecycle: porchapi.PackageRevisionLifecyclePublished, + Resources: resources, + Kptfile: kptfilev1.KptFile{ + Upstream: &kptfilev1.Upstream{}, + UpstreamLock: &kptfilev1.Locator{}, + }, + } + + extPRMap := map[repository.PackageRevisionKey]repository.PackageRevision{prKey: fakeExtPR} + inExternalOnly := []repository.PackageRevisionKey{prKey} + + // Should succeed and silently skip the invalid PR + err = repoSync.cacheExternalPRs(ctx, extPRMap, inExternalOnly) + t.Require().NoError(err, "revision=0+Published should be skipped without error") + + // PR should NOT be cached since it was skipped + prList, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + t.Require().NoError(err) + t.Empty(prList, "revision=0+Published PR should not be cached") +} + +// TestHandleInCachedOnly_DeletesPublishedCachedOnly verifies that a published workspace PR +// listed as cached-only is removed from the database while other revisions (e.g. main) remain. +func (t *DbTestSuite) TestHandleInCachedOnly_DeletesPublishedCachedOnly() { + ctx := t.Context() + externalrepo.ExternalRepoInUnitTestMode = true + + testRepo := t.createTestRepo("nodelete-ns", "nodelete-repo") + defer t.deleteTestRepo(testRepo.Key()) + + mockCache := mockcachetypes.NewMockCache(t.T()) + cachetypes.CacheInstance = mockCache + mockCache.EXPECT().GetRepository(mock.Anything).Return(testRepo).Maybe() + + err := testRepo.OpenRepository(ctx, externalrepotypes.ExternalRepoOptions{}) + t.Require().NoError(err) + defer func() { + if err := testRepo.Close(ctx); err != nil { + t.T().Logf("Failed to close test repo: %v", err) + } + }() + + repoSync := &repositorySync{ + repo: testRepo, + } + + // Create and publish a PR so it appears in the "cached only" set + newPRDef := porchapi.PackageRevision{ + Spec: porchapi.PackageRevisionSpec{ + RepositoryName: "nodelete-repo", + PackageName: "my-pkg", + WorkspaceName: "my-ws", + Lifecycle: porchapi.PackageRevisionLifecyclePublished, + }, + } + prDraft, err := testRepo.CreatePackageRevisionDraft(ctx, &newPRDef) + t.Require().NoError(err) + + dbPR, err := testRepo.ClosePackageRevisionDraft(ctx, prDraft, 0) + t.Require().NoError(err) + + err = dbPR.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecycleProposed) + t.Require().NoError(err) + + dbPR, err = testRepo.ClosePackageRevisionDraft(ctx, dbPR.(repository.PackageRevisionDraft), 0) + t.Require().NoError(err) + + err = dbPR.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecyclePublished) + t.Require().NoError(err) + + dbPR, err = testRepo.ClosePackageRevisionDraft(ctx, dbPR.(repository.PackageRevisionDraft), 0) + t.Require().NoError(err) + + prList, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + t.Require().NoError(err) + t.Len(prList, 2, "PR should exist before calling handleInCachedOnly (workspace PR + main branch PR created on publish)") + + cachedPrMap := repository.PrSlice2Map(prList) + inCachedOnly := []repository.PackageRevisionKey{dbPR.Key()} + + // Call handleInCachedOnly – should delete the cached-only workspace PR. + err = repoSync.handleInCachedOnly(ctx, cachedPrMap, inCachedOnly) + t.Require().NoError(err) + + prListAfter, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + t.Require().NoError(err) + t.Len(prListAfter, 1, "cached-only published workspace PR should be deleted; main branch revision remains") +} + +// TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse verifies that a Draft PR +// that has not been pushed to git is not deleted when pushDraftsToGit=false. +func (t *DbTestSuite) TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse() { + ctx := t.Context() + externalrepo.ExternalRepoInUnitTestMode = true + + testRepo := t.createTestRepo("draftkeep-ns", "draftkeep-repo") + defer t.deleteTestRepo(testRepo.Key()) + + mockCache := mockcachetypes.NewMockCache(t.T()) + cachetypes.CacheInstance = mockCache + mockCache.EXPECT().GetRepository(mock.Anything).Return(testRepo).Maybe() + + err := testRepo.OpenRepository(ctx, externalrepotypes.ExternalRepoOptions{}) + t.Require().NoError(err) + defer func() { + if err := testRepo.Close(ctx); err != nil { + t.T().Logf("Failed to close test repo: %v", err) + } + }() + + repoSync := &repositorySync{ + repo: testRepo, + // pushDraftsToGit is false (default zero value) + } + + // Create a Draft PR (lastPushedDbUpdated will be nil) + newPRDef := porchapi.PackageRevision{ + Spec: porchapi.PackageRevisionSpec{ + RepositoryName: "draftkeep-repo", + PackageName: "draft-pkg", + WorkspaceName: "draft-ws", + Lifecycle: porchapi.PackageRevisionLifecycleDraft, + }, + } + prDraft, err := testRepo.CreatePackageRevisionDraft(ctx, &newPRDef) + t.Require().NoError(err) + + dbPR, err := testRepo.ClosePackageRevisionDraft(ctx, prDraft, 0) + t.Require().NoError(err) + + // Verify there is a draft PR in the DB + prList, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Lifecycles: []porchapi.PackageRevisionLifecycle{porchapi.PackageRevisionLifecycleDraft}, + }) + t.Require().NoError(err) + t.Len(prList, 1, "Draft PR should exist before calling handleInCachedOnly") + + // Build cachedPrMap directly with the dbPackageRevision (which has not been pushed yet) + cachedPRTyped := dbPR.(*dbPackageRevision) + t.False(hasBeenPushedToGit(cachedPRTyped), "new draft should not have been pushed to git") + + cachedPrMap := map[repository.PackageRevisionKey]repository.PackageRevision{ + dbPR.Key(): cachedPRTyped, + } + inCachedOnly := []repository.PackageRevisionKey{dbPR.Key()} + + // Should not delete the Draft PR since pushDraftsToGit=false means it is kept + err = repoSync.handleInCachedOnly(ctx, cachedPrMap, inCachedOnly) + t.Require().NoError(err) + + prListAfter, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ + Lifecycles: []porchapi.PackageRevisionLifecycle{porchapi.PackageRevisionLifecycleDraft}, + }) + t.Require().NoError(err) + t.Len(prListAfter, 1, "unpushed Draft PR should not be deleted when pushDraftsToGit=false") +} + +// TestDeleteCachedOnlyPR_AlreadyDeleted verifies that deleteCachedOnlyPR returns nil (no error) +// when the PR no longer exists in the database (sql.ErrNoRows path). +func (t *DbTestSuite) TestDeleteCachedOnlyPR_AlreadyDeleted() { + ctx := t.Context() + externalrepo.ExternalRepoInUnitTestMode = true + + testRepo := t.createTestRepo("alreadydel-ns", "alreadydel-repo") + defer t.deleteTestRepo(testRepo.Key()) + + mockCache := mockcachetypes.NewMockCache(t.T()) + cachetypes.CacheInstance = mockCache + mockCache.EXPECT().GetRepository(mock.Anything).Return(testRepo).Maybe() + + err := testRepo.OpenRepository(ctx, externalrepotypes.ExternalRepoOptions{}) + t.Require().NoError(err) + defer func() { + if err := testRepo.Close(ctx); err != nil { + t.T().Logf("Failed to close test repo: %v", err) + } + }() + + repoSync := &repositorySync{repo: testRepo} + + // Key for a PR that has never been persisted to the DB + nonExistentKey := repository.PackageRevisionKey{ + PkgKey: repository.PackageKey{ + RepoKey: testRepo.Key(), + Package: "nonexistent-pkg", + }, + Revision: 99, + WorkspaceName: "nonexistent-ws", + } + snapshot := &dbPackageRevision{pkgRevKey: nonExistentKey} + + // Should return nil because pkgRevReadFromDB returns sql.ErrNoRows + err = repoSync.deleteCachedOnlyPR(ctx, nonExistentKey, snapshot) + t.Require().NoError(err, "should not error when PR is already absent from DB") +} + +// TestDeleteCachedOnlyPR_ChangedSinceSnapshot verifies that deleteCachedOnlyPR skips +// deletion when the PR was updated after the cached snapshot was taken. +func (t *DbTestSuite) TestDeleteCachedOnlyPR_ChangedSinceSnapshot() { + ctx := t.Context() + externalrepo.ExternalRepoInUnitTestMode = true + + testRepo := t.createTestRepo("changed-ns", "changed-repo") + defer t.deleteTestRepo(testRepo.Key()) + + mockCache := mockcachetypes.NewMockCache(t.T()) + cachetypes.CacheInstance = mockCache + mockCache.EXPECT().GetRepository(mock.Anything).Return(testRepo).Maybe() + + err := testRepo.OpenRepository(ctx, externalrepotypes.ExternalRepoOptions{}) + t.Require().NoError(err) + defer func() { + if err := testRepo.Close(ctx); err != nil { + t.T().Logf("Failed to close test repo: %v", err) + } + }() + + repoSync := &repositorySync{ + repo: testRepo, + } + + // Create and publish a PR + newPRDef := porchapi.PackageRevision{ + Spec: porchapi.PackageRevisionSpec{ + RepositoryName: "changed-repo", + PackageName: "changed-pkg", + WorkspaceName: "changed-ws", + Lifecycle: porchapi.PackageRevisionLifecyclePublished, + }, + } + prDraft, err := testRepo.CreatePackageRevisionDraft(ctx, &newPRDef) + t.Require().NoError(err) + + dbPR, err := testRepo.ClosePackageRevisionDraft(ctx, prDraft, 0) + t.Require().NoError(err) + + err = dbPR.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecycleProposed) + t.Require().NoError(err) + + dbPR, err = testRepo.ClosePackageRevisionDraft(ctx, dbPR.(repository.PackageRevisionDraft), 0) + t.Require().NoError(err) + + err = dbPR.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecyclePublished) + t.Require().NoError(err) + + dbPR, err = testRepo.ClosePackageRevisionDraft(ctx, dbPR.(repository.PackageRevisionDraft), 0) + t.Require().NoError(err) + + prList, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + t.Require().NoError(err) + t.Len(prList, 2, "PR should exist in DB (workspace PR + main branch PR created on publish)") + + // Construct a STALE snapshot with a timestamp in the past (different from current updated) + staleSnapshot := &dbPackageRevision{ + pkgRevKey: dbPR.Key(), + updated: time.Now().Add(-1 * time.Hour), // stale - differs from actual DB timestamp + lifecycle: porchapi.PackageRevisionLifecyclePublished, + } + + // deleteCachedOnlyPR should skip deletion because the snapshot is stale + err = repoSync.deleteCachedOnlyPR(ctx, dbPR.Key(), staleSnapshot) + t.Require().NoError(err, "stale snapshot should cause deletion to be skipped without error") + + // PR should still exist in the DB + prListAfter, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + t.Require().NoError(err) + t.Len(prListAfter, 2, "PR should not be deleted when snapshot is stale") +} diff --git a/pkg/cache/dbcache/util.go b/pkg/cache/dbcache/util.go index 2ffbe4477..f85328bb3 100644 --- a/pkg/cache/dbcache/util.go +++ b/pkg/cache/dbcache/util.go @@ -17,10 +17,16 @@ package dbcache import ( "encoding/json" "os/user" + "sync" + kptfile "github.com/kptdev/kpt/api/kptfile/v1" + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/pkg/repository" "k8s.io/klog/v2" ) +const unpushedGitCommit = "not-pushed" + func getCurrentUser() string { currentUser, err := user.Current() if err == nil { @@ -40,7 +46,122 @@ func valueAsJSON(value any) string { } func setValueFromJSON(jsonValue string, value any) { - if err := json.Unmarshal([]byte(jsonValue), &value); err != nil { + if err := json.Unmarshal([]byte(jsonValue), value); err != nil { klog.Errorf("unmarshal of json value %v failed, %v ", jsonValue, err) } } + +type keyedMutex struct { + mu sync.Mutex + refs int +} + +type lockManager struct { + mu sync.Mutex + locks map[string]*keyedMutex +} + +var globalLockManager = &lockManager{ + locks: make(map[string]*keyedMutex), +} + +func (lm *lockManager) lockKey(key string) { + lm.mu.Lock() + km, exists := lm.locks[key] + if !exists { + km = &keyedMutex{} + lm.locks[key] = km + } + km.refs++ + lm.mu.Unlock() + + km.mu.Lock() +} + +func (lm *lockManager) unlockKey(key string) { + lm.mu.Lock() + km := lm.locks[key] + km.mu.Unlock() + km.refs-- + if km.refs == 0 { + delete(lm.locks, key) + } + lm.mu.Unlock() +} + +func lockRepoKey(repoKey repository.RepositoryKey) { + globalLockManager.lockKey(repoKey.String()) +} + +func unlockRepoKey(repoKey repository.RepositoryKey) { + globalLockManager.unlockKey(repoKey.String()) +} + +func lockPkgKey(pkgKey repository.PackageKey) { + globalLockManager.lockKey(pkgKey.String()) +} + +func unlockPkgKey(pkgKey repository.PackageKey) { + globalLockManager.unlockKey(pkgKey.String()) +} + +func extPRCommit(pr *dbPackageRevision) string { + return extPRCommitFromLocator(pr.extPRID) +} + +func extPRCommitFromLocator(loc kptfile.Locator) string { + if loc.Git != nil { + return loc.Git.Commit + } + return "" +} + +func hasBeenPushedToGit(pr *dbPackageRevision) bool { + if pr.lastPushedDbUpdated != nil { + return true + } + commit := extPRCommit(pr) + return commit != "" && commit != unpushedGitCommit +} + +func dbContentChangedSincePush(pr *dbPackageRevision) bool { + if pr.lastPushedDbUpdated == nil { + return true + } + return !pr.updated.Equal(*pr.lastPushedDbUpdated) +} + +func commitTaskForPush(pr *dbPackageRevision, existingInGit bool) *porchapi.Task { + if existingInGit || hasBeenPushedToGit(pr) { + return &porchapi.Task{Type: porchapi.TaskTypePush} + } + + for i := range pr.tasks { + if porchapi.IsValidFirstTaskType(pr.tasks[i].Type) { + return &pr.tasks[i] + } + } + + return nil +} + +func commitTaskForPublishedPush(tasks []porchapi.Task, existingInGit bool) *porchapi.Task { + if existingInGit { + return &porchapi.Task{Type: porchapi.TaskTypePush} + } + + for i := range tasks { + if porchapi.IsValidFirstTaskType(tasks[i].Type) { + return &tasks[i] + } + } + + return &porchapi.Task{Type: porchapi.TaskTypePush} +} + +func prNeedsPushToGit(pr *dbPackageRevision) bool { + if pr.lastPushedDbUpdated == nil { + return true + } + return !pr.lastPushedDbUpdated.Equal(pr.updated) +} diff --git a/pkg/cache/dbcache/util_test.go b/pkg/cache/dbcache/util_test.go index 19f4e95a4..54d6a681c 100644 --- a/pkg/cache/dbcache/util_test.go +++ b/pkg/cache/dbcache/util_test.go @@ -15,7 +15,12 @@ package dbcache import ( + "os/user" "time" + + kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/pkg/repository" ) func (t *DbTestSuite) TestUtil() { @@ -27,3 +32,232 @@ func (t *DbTestSuite) TestUtil() { setValueFromJSON("", &secondValue) t.Equal(time.Second, secondValue) } + +func (t *DbTestSuite) TestGetCurrentUser() { + got := getCurrentUser() + t.NotEmpty(got) + + if u, err := user.Current(); err == nil { + t.Equal(u.Username, got) + } +} + +func (t *DbTestSuite) TestValueAsJSONAndSetValueFromJSON() { + type myStruct struct { + Name string `json:"name"` + Value int `json:"value"` + } + + original := myStruct{Name: "test", Value: 42} + jsonStr := valueAsJSON(original) + t.NotEmpty(jsonStr) + + var restored myStruct + setValueFromJSON(jsonStr, &restored) + t.Equal(original, restored) +} + +func (t *DbTestSuite) TestValueAsJSONInvalidInput() { + ch := make(chan int) + jsonStr := valueAsJSON(ch) + t.Equal("", jsonStr) +} + +func (t *DbTestSuite) TestSetValueFromJSONInvalidInput() { + original := 99 + setValueFromJSON("not-valid-json{{{", &original) + t.Equal(99, original) +} + +func (t *DbTestSuite) TestLockManagerLockKey() { + lm := &lockManager{locks: make(map[string]*keyedMutex)} + + lm.lockKey("key1") + km1 := lm.locks["key1"] + t.NotNil(km1) + t.Equal(1, km1.refs) + lm.unlockKey("key1") + t.Len(lm.locks, 0) + + lm.lockKey("key2") + t.Len(lm.locks, 1) + lm.unlockKey("key2") +} + +func (t *DbTestSuite) TestLockManagerWaitsOnSameMutex() { + lm := &lockManager{locks: make(map[string]*keyedMutex)} + const key = "concurrent-key" + + lm.lockKey(key) + km := lm.locks[key] + + secondAcquired := make(chan struct{}) + go func() { + lm.lockKey(key) + close(secondAcquired) + lm.unlockKey(key) + }() + + t.Eventually(func() bool { + return lm.locks[key] == km && km.refs == 2 + }, time.Second, 5*time.Millisecond) + + lm.unlockKey(key) + + <-secondAcquired + t.Len(lm.locks, 0) +} + +func (t *DbTestSuite) TestLockRepoKey() { + repoKey := repository.RepositoryKey{Namespace: "ns", Name: "repo"} + lockRepoKey(repoKey) + t.Len(globalLockManager.locks, 1) + unlockRepoKey(repoKey) + t.Len(globalLockManager.locks, 0) +} + +func (t *DbTestSuite) TestLockPkgKey() { + pkgKey := repository.PackageKey{ + RepoKey: repository.RepositoryKey{Namespace: "ns", Name: "repo"}, + Package: "my-pkg", + } + + lockPkgKey(pkgKey) + t.Len(globalLockManager.locks, 1) + unlockPkgKey(pkgKey) + t.Len(globalLockManager.locks, 0) +} + +func (t *DbTestSuite) TestExtPRCommit() { + t.Equal("", extPRCommit(&dbPackageRevision{})) + t.Equal("abc123", extPRCommit(&dbPackageRevision{ + extPRID: kptfilev1.Locator{Git: &kptfilev1.GitLock{Commit: "abc123"}}, + })) + t.Equal(unpushedGitCommit, extPRCommit(&dbPackageRevision{ + extPRID: kptfilev1.Locator{Git: &kptfilev1.GitLock{Commit: unpushedGitCommit}}, + })) +} + +func (t *DbTestSuite) TestHasBeenPushedToGit() { + now := time.Now() + t.False(hasBeenPushedToGit(&dbPackageRevision{})) + t.True(hasBeenPushedToGit(&dbPackageRevision{lastPushedDbUpdated: &now})) + t.True(hasBeenPushedToGit(&dbPackageRevision{ + extPRID: kptfilev1.Locator{Git: &kptfilev1.GitLock{Commit: "abc123"}}, + })) + t.False(hasBeenPushedToGit(&dbPackageRevision{ + extPRID: kptfilev1.Locator{Git: &kptfilev1.GitLock{Commit: unpushedGitCommit}}, + })) +} + +func (t *DbTestSuite) TestDbContentChangedSincePush() { + now := time.Now() + + pr := &dbPackageRevision{updated: now} + t.True(dbContentChangedSincePush(pr)) + + pr.lastPushedDbUpdated = &now + t.False(dbContentChangedSincePush(pr)) + + later := now.Add(time.Second) + pr.updated = later + t.True(dbContentChangedSincePush(pr)) +} + +func (t *DbTestSuite) TestCommitTaskForPush() { + now := time.Now() + pr := &dbPackageRevision{lastPushedDbUpdated: &now} + task := commitTaskForPush(pr, false) + t.Require().NotNil(task) + t.Equal(porchapi.TaskTypePush, task.Type) + + prWithExtCommit := &dbPackageRevision{ + extPRID: kptfilev1.Locator{Git: &kptfilev1.GitLock{Commit: "abc123"}}, + tasks: []porchapi.Task{{Type: porchapi.TaskTypeEdit}}, + } + taskExt := commitTaskForPush(prWithExtCommit, false) + t.Require().NotNil(taskExt) + t.Equal(porchapi.TaskTypePush, taskExt.Type) + + prExistingInGit := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeEdit}}} + taskExisting := commitTaskForPush(prExistingInGit, true) + t.Require().NotNil(taskExisting) + t.Equal(porchapi.TaskTypePush, taskExisting.Type) + + pr2 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeRender}}} + t.Nil(commitTaskForPush(pr2, false)) + + pr3 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeInit}}} + task3 := commitTaskForPush(pr3, false) + t.Require().NotNil(task3) + t.Equal(porchapi.TaskTypeInit, task3.Type) + + pr4 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeClone}}} + task4 := commitTaskForPush(pr4, false) + t.Require().NotNil(task4) + t.Equal(porchapi.TaskTypeClone, task4.Type) + + pr5 := &dbPackageRevision{} + t.Nil(commitTaskForPush(pr5, false)) +} + +func (t *DbTestSuite) TestCommitTaskForPublishedPush() { + editTasks := []porchapi.Task{{Type: porchapi.TaskTypeEdit}} + + task := commitTaskForPublishedPush(editTasks, true) + t.Require().NotNil(task) + t.Equal(porchapi.TaskTypePush, task.Type) + + taskNew := commitTaskForPublishedPush(editTasks, false) + t.Require().NotNil(taskNew) + t.Equal(porchapi.TaskTypeEdit, taskNew.Type) + + taskDefault := commitTaskForPublishedPush(nil, false) + t.Require().NotNil(taskDefault) + t.Equal(porchapi.TaskTypePush, taskDefault.Type) +} + +func (t *DbTestSuite) TestPrNeedsPushToGit() { + now := time.Now() + + pr := &dbPackageRevision{} + t.True(prNeedsPushToGit(pr)) + + pr.updated = now + pr.lastPushedDbUpdated = &now + t.False(prNeedsPushToGit(pr)) + + later := now.Add(time.Second) + pr.updated = later + t.True(prNeedsPushToGit(pr)) +} + +func (t *DbTestSuite) TestPreservePushMarkersIfUnset() { + dbUpdated := time.Now() + + existing := &dbPackageRevision{ + lastPushedDbUpdated: &dbUpdated, + extPRID: kptfilev1.Locator{ + Git: &kptfilev1.GitLock{Commit: "abc123"}, + }, + } + + pr := &dbPackageRevision{ + extPRID: kptfilev1.Locator{ + Git: &kptfilev1.GitLock{Commit: unpushedGitCommit}, + }, + } + preservePushMarkersIfUnset(pr, existing) + t.Require().NotNil(pr.lastPushedDbUpdated) + t.Equal(dbUpdated, *pr.lastPushedDbUpdated) + t.Equal("abc123", extPRCommit(pr)) + + otherUpdated := dbUpdated.Add(time.Minute) + prWithMarker := &dbPackageRevision{lastPushedDbUpdated: &otherUpdated} + preservePushMarkersIfUnset(prWithMarker, existing) + t.Equal(otherUpdated, *prWithMarker.lastPushedDbUpdated) + + prEmpty := &dbPackageRevision{} + preservePushMarkersIfUnset(prEmpty, &dbPackageRevision{}) + t.Nil(prEmpty.lastPushedDbUpdated) +} diff --git a/pkg/engine/pushpr.go b/pkg/engine/pushpr.go deleted file mode 100644 index 0b8b40261..000000000 --- a/pkg/engine/pushpr.go +++ /dev/null @@ -1,174 +0,0 @@ -// Copyright 2025 The kpt Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package engine - -import ( - "context" - "fmt" - - kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" - porchapi "github.com/kptdev/porch/api/porch/v1alpha1" - "github.com/kptdev/porch/pkg/repository" - pctx "github.com/kptdev/porch/pkg/util/context" - pkgerrors "github.com/pkg/errors" - "go.opentelemetry.io/otel/trace" - "k8s.io/klog/v2" -) - -func PushPackageRevision(ctx context.Context, repo repository.Repository, pr repository.PackageRevision, pushDraftsToGit bool, gitPR repository.PackageRevision) (kptfilev1.Locator, error) { - ctx, span := tracer.Start(ctx, "PushPackageRevision", trace.WithAttributes()) - defer span.End() - - prName := repository.ComposePkgRevObjName(pr.Key()) - if pushDraftsToGit { - klog.InfoS("[Engine] Pushing PackageRevision to repository and to Git for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - } else { - klog.InfoS("[Engine] Pushing PackageRevision to repository for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - } - defer func() { - klog.V(3).InfoS("[Engine] Push PackageRevision to repository completed for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - }() - - prLifecycle := pr.Lifecycle(ctx) - if prLifecycle != porchapi.PackageRevisionLifecyclePublished { - return kptfilev1.Locator{}, fmt.Errorf("cannot push package revision %+v, package revision lifecycle is %q, it should be \"Published\"", pr.Key(), prLifecycle) - } - - apiPr, err := pr.GetPackageRevision(ctx) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get API definition:", pr.Key(), repo.Key()) - } - - resources, err := pr.GetResources(ctx) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get package revision resources:", pr.Key(), repo.Key()) - } - - var draft repository.PackageRevisionDraft - var foundExisting bool - - if pushDraftsToGit { - if gitPR != nil { - klog.V(3).InfoS("[Engine] Updating existing Git draft for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - draft, err = repo.UpdatePackageRevision(ctx, gitPR) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update git PR:", pr.Key(), repo.Key()) - } - foundExisting = true - } else { - klog.V(3).InfoS("[Engine] Listing existing Git revisions for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - existingPRs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ - Key: repository.PackageRevisionKey{ - PkgKey: pr.Key().PkgKey, - WorkspaceName: pr.Key().WorkspaceName, - }, - }) - - if err == nil && len(existingPRs) > 0 { - klog.V(3).InfoS("[Engine] Updating existing Git package revision for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - draft, err = repo.UpdatePackageRevision(ctx, existingPRs[0]) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update existing package revision:", pr.Key(), repo.Key()) - } - foundExisting = true - } - } - } - - if !foundExisting { - draft, err = repo.CreatePackageRevisionDraft(ctx, apiPr) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not create package revision draft:", pr.Key(), repo.Key()) - } - } - - if !foundExisting { - commitTask := &porchapi.Task{Type: porchapi.TaskTypePush} - if len(apiPr.Spec.Tasks) > 0 { - commitTask = &apiPr.Spec.Tasks[0] - } - - if err = draft.UpdateResources(ctx, resources, commitTask); err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update package revision resources:", pr.Key(), repo.Key()) - } - } - - if err = draft.UpdateLifecycle(ctx, porchapi.PackageRevisionLifecyclePublished); err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update package revision draft lifecycle to \"Published\":", pr.Key(), repo.Key()) - } - - pushedPR, err := repo.ClosePackageRevisionDraft(ctx, draft, pr.Key().Revision) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not close package revision draft:", pr.Key(), repo.Key()) - } - - _, pushedPRUpstreamLock, err := pushedPR.GetLock(ctx) - if err != nil { - return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "read of upstream lock for package revision %+v pushed to repository %+v failed", pr.Key(), repo.Key()) - } - - return pushedPRUpstreamLock, nil -} - -func GetOrCreateGitDraft(ctx context.Context, repo repository.Repository, pr repository.PackageRevision, gitPR repository.PackageRevision) (draft repository.PackageRevisionDraft, updatedGitPR repository.PackageRevision, err error) { - prName := repository.ComposePkgRevObjName(pr.Key()) - klog.InfoS("[Engine] Getting or creating Git draft for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - defer func() { - klog.V(3).InfoS("[Engine] Get or create Git draft completed for PackageRevision", - pctx.LogMetadataFromWithExtras(ctx, "packageRevision", prName)...) - }() - - if gitPR != nil { - gitDraft, err := repo.UpdatePackageRevision(ctx, gitPR) - if err != nil { - return nil, nil, pkgerrors.Wrapf(err, "failed to update git PR for %+v", pr.Key()) - } - return gitDraft, gitPR, nil - } - - existingPRs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{ - Key: repository.PackageRevisionKey{ - PkgKey: pr.Key().PkgKey, - WorkspaceName: pr.Key().WorkspaceName, - }, - }) - - if err == nil && len(existingPRs) > 0 { - gitDraft, err := repo.UpdatePackageRevision(ctx, existingPRs[0]) - if err != nil { - return nil, nil, pkgerrors.Wrapf(err, "failed to update existing git branch for %+v", pr.Key()) - } - return gitDraft, existingPRs[0], nil - } - - apiPr, err := pr.GetPackageRevision(ctx) - if err != nil { - return nil, nil, pkgerrors.Wrapf(err, "failed to get API representation for %+v", pr.Key()) - } - - gitDraft, err := repo.CreatePackageRevisionDraft(ctx, apiPr) - if err != nil { - return nil, nil, pkgerrors.Wrapf(err, "failed to create git draft for %+v", pr.Key()) - } - - return gitDraft, nil, nil -} diff --git a/pkg/externalrepo/fake/repository.go b/pkg/externalrepo/fake/repository.go index d15cb2a51..d0dac9359 100644 --- a/pkg/externalrepo/fake/repository.go +++ b/pkg/externalrepo/fake/repository.go @@ -90,8 +90,11 @@ func (r *Repository) DeletePackageRevision(context.Context, repository.PackageRe return nil } -func (r *Repository) UpdatePackageRevision(context.Context, repository.PackageRevision) (repository.PackageRevisionDraft, error) { - return nil, nil +func (r *Repository) UpdatePackageRevision(_ context.Context, pr repository.PackageRevision) (repository.PackageRevisionDraft, error) { + if fpr, ok := pr.(*FakePackageRevision); ok { + return fpr, nil + } + return &FakePackageRevision{PrKey: pr.Key()}, nil } func (r *Repository) ListPackages(context.Context, repository.ListPackageFilter) ([]repository.Package, error) { diff --git a/pkg/externalrepo/fake/repository_test.go b/pkg/externalrepo/fake/repository_test.go index 2c9ac119b..112cf0bc4 100644 --- a/pkg/externalrepo/fake/repository_test.go +++ b/pkg/externalrepo/fake/repository_test.go @@ -90,7 +90,7 @@ func TestRepositoryFunctions(t *testing.T) { updatedPRDraft, err := fakeRepo.UpdatePackageRevision(context.TODO(), newPR) assert.Nil(t, err) - assert.Nil(t, updatedPRDraft) + assert.Equal(t, newPR, updatedPRDraft) assert.Nil(t, fakeRepo.DeletePackageRevision(context.TODO(), newPR)) diff --git a/scripts/deploy/create-deployment-blueprint.sh b/scripts/deploy/create-deployment-blueprint.sh index c98f64894..967005fbf 100755 --- a/scripts/deploy/create-deployment-blueprint.sh +++ b/scripts/deploy/create-deployment-blueprint.sh @@ -42,7 +42,7 @@ Supported Flags: --ghcr-image-prefix PREFIX ... GHCR image url prefix for running porch behind a proxy --fn-runner-warm-up-pod-cache BOOL ... disable warm-up-pod-cache in function runner --porch-cache-type TYPE ... porch cache type (CR or DB) - --db-push-drafts-to-git BOOL ... enable db-push-drafts-to-git flag for porch-server + --db-push-drafts-to-git BOOL ... enable db-push-drafts-to-git flag for porch-server and repo controller --create-v1alpha2-rpkg BOOL ... enable v1alpha2 PackageRevision CRD creation by repo controller EOF exit 1 @@ -246,6 +246,13 @@ function enable_db_push_drafts_to_git() { --match-name porch-server \ --match-namespace porch-system \ -- by-value="--db-push-drafts-to-git=false" put-value="--db-push-drafts-to-git=true" + + kpt fn eval ${DESTINATION} \ + --image ${SEARCH_REPLACE_IMG} \ + --match-kind Deployment \ + --match-name porch-controllers \ + --match-namespace porch-system \ + -- by-value="--repositories.push-drafts-to-git=false" put-value="--repositories.push-drafts-to-git=true" } function enable_v1alpha2_packagerevisions() { diff --git a/test/e2e/api/db_git_sync_test.go b/test/e2e/api/db_git_sync_test.go new file mode 100644 index 000000000..6a45499d3 --- /dev/null +++ b/test/e2e/api/db_git_sync_test.go @@ -0,0 +1,503 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package api + +import ( + "context" + "time" + + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + suiteutils "github.com/kptdev/porch/test/e2e/suiteutils" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + dbGitTestRepoName = "db-git-test-repo" + dbGitSyncWaitTimeout = 90 * time.Second +) + +func (t *PorchSuite) updatePRR(_ string, prr *porchapi.PackageRevisionResources, resourceKeys ...string) { + t.T().Helper() + t.UpdateF(prr) + + prName := prr.Name + err := wait.PollUntilContextTimeout(t.GetContext(), time.Second, dbGitSyncWaitTimeout, true, func(ctx context.Context) (bool, error) { + var latest porchapi.PackageRevisionResources + if err := t.Reader.Get(ctx, client.ObjectKey{Namespace: t.Namespace, Name: prName}, &latest); err != nil { + return false, nil + } + for _, key := range resourceKeys { + if _, ok := latest.Spec.Resources[key]; !ok { + return false, nil + } + } + if err := t.CheckRenderError(&latest.Status.RenderStatus); err != nil { + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("updatePRR: PRR %q did not reflect update (keys %v) within %v", prName, resourceKeys, dbGitSyncWaitTimeout) + } +} + +func (t *PorchSuite) triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace string) string { + t.T().Helper() + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.RequestRepoSync(repoName) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + return branchName +} + +func (t *PorchSuite) triggerRepoSyncAndWaitForNewCommit(repoName, giteaRepo, branchName, oldCommitSHA string) string { + t.T().Helper() + t.RequestRepoSync(repoName) + return t.WaitUntilGiteaBranchHasNewCommit(giteaRepo, branchName, oldCommitSHA, dbGitSyncWaitTimeout) +} + +func (t *PorchSuite) TestSyncDraftSurvivesSyncWhenInGit() { + const ( + repoName = dbGitTestRepoName + "-s1" + packageName = "pkg-survives-sync" + workspace = "v1" + giteaRepo = repoName + "-git" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s", pr.Name) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + commitSHA := t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName) + t.Logf("draft pushed to git branch %s at %s", branchName, commitSHA) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + pr = t.GetPackageRevisionWithWS(repoName, packageName, workspace) + t.Require().Equal(porchapi.PackageRevisionLifecycleDraft, pr.Spec.Lifecycle, + "draft lifecycle must remain Draft after sync") + t.Require().True(t.GiteaBranchExists(giteaRepo, branchName), + "draft git branch %q must still exist after sync", branchName) + t.Require().Equal(commitSHA, t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName), + "draft git commit must be unchanged after an in-sync reconcile") +} + +func (t *PorchSuite) TestSyncDraftSurvivesSyncWhenPushFails() { + const ( + repoName = dbGitTestRepoName + "-s2" + packageName = "pkg-branch-deleted" + workspace = "v1" + giteaRepo = repoName + "-git" + ) + + // Create a dedicated Gitea repo for this test so we can archive it in isolation. + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + // Archive the Gitea repo so any push attempt is rejected + t.SetGiteaRepoArchived(giteaRepo, true) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s (pushed_to_git=false expected because repo is archived)", pr.Name) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.Require().False(t.GiteaBranchExists(giteaRepo, branchName), + "git branch %q must NOT exist because the push was expected to fail", branchName) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + pr = t.GetPackageRevisionWithWS(repoName, packageName, workspace) + t.Require().Equal(porchapi.PackageRevisionLifecycleDraft, pr.Spec.Lifecycle) +} + +func (t *PorchSuite) TestSyncProposedAndPublishedAfterPushToGitFailed() { + const ( + repoName = dbGitTestRepoName + "-s3" + packageName = "pkg-lifecycle-recovery" + workspace = "v1" + giteaRepo = repoName + "-git" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + // Archive the Gitea repo so the push fails → pushed_to_git=false. + t.SetGiteaRepoArchived(giteaRepo, true) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s (pushed_to_git=false – repo is archived)", pr.Name) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.Require().False(t.GiteaBranchExists(giteaRepo, branchName), + "git branch %q must NOT exist because the push was expected to fail", branchName) + + // Unarchive so the re-push (triggered by resource update) can succeed. + t.SetGiteaRepoArchived(giteaRepo, false) + + // Recover by updating resources (re-pushes the branch). + var prr porchapi.PackageRevisionResources + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + prr.Spec.Resources["recovery.yaml"] = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: recovery +data: + recovered: "true" +` + t.updatePRR(repoName, &prr, "recovery.yaml") + + t.RequestRepoSync(repoName) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + t.Logf("draft branch %s recovered into git after the failed push", branchName) + + tagsBeforePublish := t.GiteaRepoTagCount(giteaRepo) + + t.GetF(client.ObjectKeyFromObject(pr), pr) + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(pr) + t.GetF(client.ObjectKeyFromObject(pr), pr) + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + published := t.UpdateApprovalF(pr) + + t.Require().NotNil(published) + t.Require().Equal(porchapi.PackageRevisionLifecyclePublished, published.Spec.Lifecycle) + t.Require().Greater(published.Spec.Revision, 0, + "published revision must have a positive revision number") + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + t.Require().Eventually(func() bool { + return t.GiteaRepoTagCount(giteaRepo) > tagsBeforePublish + }, dbGitSyncWaitTimeout, time.Second, + "published revision must create a git tag after recovery") +} + +func (t *PorchSuite) TestSyncDeleteDraftWithPushToGitFailedRemovedCleanly() { + const ( + repoName = dbGitTestRepoName + "-s4" + packageName = "pkg-delete-push-failed" + workspace = "v1" + giteaRepo = repoName + "-git" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + // Archive the Gitea repo so the push fails → pushed_to_git=false, branch never created. + t.SetGiteaRepoArchived(giteaRepo, true) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + prName := pr.Name + t.Logf("created draft %s (pushed_to_git=false – repo is archived)", prName) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.Require().False(t.GiteaBranchExists(giteaRepo, branchName), + "git branch %q must NOT exist because the push was expected to fail", branchName) + + t.DeleteF(pr) + t.Logf("deleted draft %s via Porch API", prName) + + t.WaitUntilObjectDeleted(suiteutils.PackageRevisionGVK, types.NamespacedName{Namespace: t.Namespace, Name: prName}, dbGitSyncWaitTimeout) + + t.SetGiteaRepoArchived(giteaRepo, false) + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + var list porchapi.PackageRevisionList + t.ListF(&list, client.InNamespace(t.Namespace)) + for _, item := range list.Items { + if item.Spec.RepositoryName == repoName && + item.Spec.PackageName == packageName && + item.Spec.WorkspaceName == workspace { + t.Errorf("deleted draft %s reappeared after sync", prName) + } + } +} + +func (t *PorchSuite) TestSyncConcurrentModificationNotOverwrittenByRetry() { + const ( + repoName = dbGitTestRepoName + "-s5" + packageName = "pkg-concurrent-mod" + workspace = "v1" + giteaRepo = repoName + "-git" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + // Archive the Gitea repo so the push fails → pushed_to_git=false, async retry pending. + t.SetGiteaRepoArchived(giteaRepo, true) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s (pushed_to_git=false – async retry now pending)", pr.Name) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.Require().False(t.GiteaBranchExists(giteaRepo, branchName), + "git branch %q must NOT exist because the push was expected to fail", branchName) + + // Update resources while the retry is pending + var prr porchapi.PackageRevisionResources + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + const newFileKey = "concurrent-update.yaml" + prr.Spec.Resources[newFileKey] = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: concurrent-update +data: + updated-by: concurrent-test +` + t.updatePRR(repoName, &prr, newFileKey) + t.Logf("updated resources for draft %s (advances updated timestamp)", pr.Name) + + t.SetGiteaRepoArchived(giteaRepo, false) + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + pr = t.GetPackageRevisionWithWS(repoName, packageName, workspace) + t.Require().Equal(porchapi.PackageRevisionLifecycleDraft, pr.Spec.Lifecycle) + + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + _, hasConcurrentUpdate := prr.Spec.Resources[newFileKey] + t.Require().True(hasConcurrentUpdate, + "draft resources must contain concurrent update file %q; stale retry must not overwrite", newFileKey) +} + +func (t *PorchSuite) TestSyncDoesNotPullGitChangeIntoDB() { + const ( + repoName = dbGitTestRepoName + "-s6" + packageName = "pkg-git-pull" + workspace = "v1" + giteaRepo = repoName + "-git" + newFileKey = "from-git.yaml" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s", pr.Name) + + branchName := t.triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace) + + // Commit a new file directly to the git branch, bypassing Porch. + newFileContent := `apiVersion: v1 +kind: ConfigMap +metadata: + name: from-git +data: + source: direct-git-commit +` + + gitFilePath := packageName + "/" + newFileKey + t.GiteaCommitFileToBranch(giteaRepo, branchName, gitFilePath, newFileContent, "add from-git.yaml via test") + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + var prr porchapi.PackageRevisionResources + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + _, hasFromGit := prr.Spec.Resources[newFileKey] + t.Require().False(hasFromGit, + "package resources must NOT contain %q – push-only sync does not pull git changes into DB", newFileKey) +} + +func (t *PorchSuite) TestSyncPublishedPackageCachedFromExternalRepo() { + const ( + repoName = dbGitTestRepoName + "-s7" + packageName = "pkg-git-cache" + workspace = "v1" + giteaRepo = repoName + "-git" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + secretName := t.CreateOrUpdateSecret(repoName, t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + // First porch registration: create manually so we can delete mid-test + // without triggering a test failure from the cleanup registered by + // RegisterGitRepositoryF. + repo1 := t.BuildGitRepoObject(repoName, repoURL, secretName) + // Safety-net cleanup in case the test fails before the explicit delete below. + t.Cleanup(func() { + t.DeleteL(repo1) + t.WaitUntilRepositoryDeleted(repoName, t.Namespace) + t.WaitUntilAllPackagesDeleted(repoName, t.Namespace) + }) + t.CreateF(repo1) + t.WaitUntilRepositoryReady(repoName, t.Namespace) + + // Create and publish a package so a git tag is created. + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.GetF(client.ObjectKeyFromObject(pr), pr) + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed + t.UpdateF(pr) + t.GetF(client.ObjectKeyFromObject(pr), pr) + pr.Spec.Lifecycle = porchapi.PackageRevisionLifecyclePublished + published := t.UpdateApprovalF(pr) + t.Require().NotNil(published) + t.Require().Equal(porchapi.PackageRevisionLifecyclePublished, published.Spec.Lifecycle) + publishedRevision := published.Spec.Revision + + // Delete the first porch registration. This clears the DB cache while + // the git tag remains in the Gitea repository. + t.DeleteF(repo1) + t.WaitUntilRepositoryDeleted(repoName, t.Namespace) + t.WaitUntilAllPackagesDeleted(repoName, t.Namespace) + + // Second porch registration pointing at the same git repo. + // The initial sync will call cacheExternalPRs for the published tag. + repo2 := t.BuildGitRepoObject(repoName, repoURL, secretName) + t.Cleanup(func() { + t.DeleteL(repo2) + t.WaitUntilRepositoryDeleted(repoName, t.Namespace) + t.WaitUntilAllPackagesDeleted(repoName, t.Namespace) + }) + t.CreateF(repo2) + // WaitUntilRepositoryReady blocks until the first sync completes, during + // which cacheExternalPRs should have stored the published revision. + t.WaitUntilRepositoryReady(repoName, t.Namespace) + + restoredPR := t.WaitUntilPackageRevisionExists(repoName, packageName, publishedRevision) + t.Require().Equal(porchapi.PackageRevisionLifecyclePublished, restoredPR.Spec.Lifecycle, + "published package revision must be re-cached from the git tag after re-registration") +} + +func (t *PorchSuite) TestSyncReconcilesDBChangedAndPushesToGit() { + const ( + repoName = dbGitTestRepoName + "-s8" + packageName = "pkg-db-push" + workspace = "v1" + giteaRepo = repoName + "-git" + newFileKey = "db-update.yaml" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s", pr.Name) + + branchName := t.triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace) + initialSHA := t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName) + t.Logf("initial branch commit SHA: %s", initialSHA) + + // Archive the repo so the next push attempt fails. + t.SetGiteaRepoArchived(giteaRepo, true) + + // Update resources – the push triggered by this update will fail, leaving + // the DB updated (updated > lastPushedDbUpdated) while git stays at initialSHA. + var prr porchapi.PackageRevisionResources + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + prr.Spec.Resources[newFileKey] = `apiVersion: v1 +kind: ConfigMap +metadata: + name: db-update +data: + updated-by: reconcile-test +` + t.updatePRR(repoName, &prr, newFileKey) + t.Logf("updated resources for draft %s (push expected to fail – repo is archived)", pr.Name) + + // Unarchive and trigger sync. handleInBoth detects dbChanged and enqueues a push. + t.SetGiteaRepoArchived(giteaRepo, false) + newSHA := t.triggerRepoSyncAndWaitForNewCommit(repoName, giteaRepo, branchName, initialSHA) + t.Logf("branch advanced from %s to %s after reconcile push", initialSHA, newSHA) + + pr = t.GetPackageRevisionWithWS(repoName, packageName, workspace) + t.Require().Equal(porchapi.PackageRevisionLifecycleDraft, pr.Spec.Lifecycle) + + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + _, hasUpdate := prr.Spec.Resources[newFileKey] + t.Require().True(hasUpdate, + "draft resources must still contain %q after the reconcile pushed DB content to git", newFileKey) +} + +func (t *PorchSuite) TestSyncBothChangedDBWins() { + const ( + repoName = dbGitTestRepoName + "-s9" + packageName = "pkg-db-wins" + workspace = "v1" + giteaRepo = repoName + "-git" + dbFileKey = "db-side.yaml" + gitFileKey = "git-side.yaml" + ) + + repoURL := t.CreateGiteaRepo(giteaRepo) + t.RegisterGitRepositoryF(repoURL, repoName, "", t.GiteaUser, suiteutils.Password(t.GiteaPassword)) + + pr := t.CreatePackageDraftF(repoName, packageName, workspace) + t.Logf("created draft %s", pr.Name) + + branchName := t.triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace) + initialSHA := t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName) + t.Logf("initial branch commit SHA: %s", initialSHA) + + // Archive the repo so pushes fail. + t.SetGiteaRepoArchived(giteaRepo, true) + + // Change #1: update resources in the DB. The resulting push fails, so the + // DB advances (updated > lastPushedDbUpdated) while git stays at initialSHA. + var prr porchapi.PackageRevisionResources + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + prr.Spec.Resources[dbFileKey] = `apiVersion: v1 +kind: ConfigMap +metadata: + name: db-side +data: + origin: db +` + t.updatePRR(repoName, &prr, dbFileKey) + t.Logf("updated resources in DB (push expected to fail – repo is archived)") + + // Change #2: commit a different file directly to the git branch. Sync should + // push DB content to git without pulling the git-side change into the DB. + t.SetGiteaRepoArchived(giteaRepo, false) + gitFilePath := packageName + "/" + gitFileKey + t.GiteaCommitFileToBranch(giteaRepo, branchName, gitFilePath, + `apiVersion: v1 +kind: ConfigMap +metadata: + name: git-side +data: + origin: git +`, "add git-side.yaml via test") + externalOnlySHA := t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName) + t.Logf("git-only commit SHA: %s", externalOnlySHA) + + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + prr.Spec.Resources[dbFileKey] = `apiVersion: v1 +kind: ConfigMap +metadata: + name: db-side +data: + origin: db-after-git +` + t.updatePRR(repoName, &prr, dbFileKey) + + t.triggerRepoSyncAndWaitForNewCommit(repoName, giteaRepo, branchName, externalOnlySHA) + + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) + _, hasDBFile := prr.Spec.Resources[dbFileKey] + t.Require().True(hasDBFile, + "draft resources must contain %q (DB-side change) after DB wins reconcile", dbFileKey) + + _, hasGitFile := prr.Spec.Resources[gitFileKey] + t.Require().False(hasGitFile, + "draft resources must NOT contain %q (git-side change) – DB wins and git content is not pulled in", gitFileKey) +} diff --git a/test/e2e/crd/lifecycle_test.go b/test/e2e/crd/lifecycle_test.go index 20a4a0e2e..5d11971aa 100644 --- a/test/e2e/crd/lifecycle_test.go +++ b/test/e2e/crd/lifecycle_test.go @@ -201,7 +201,7 @@ var _ = Describe("Lifecycle", Ordered, Label("lifecycle"), func() { // Known gap: dbPackageRevision.Delete only calls git delete for Published // packages. Draft/proposed branches are not cleaned up. Tracked as Issue 36. - PIt("should clean up draft git branch when a Draft package is deleted", func() { + It("should clean up draft git branch when a Draft package is deleted", func() { By("creating a draft package") pr := newPackageRevision(env.Namespace, env.RepoName, "del-draft-br", "v1", withInit("draft branch cleanup")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) @@ -225,7 +225,7 @@ var _ = Describe("Lifecycle", Ordered, Label("lifecycle"), func() { }).WithTimeout(defaultTimeout).Should(Not(ContainElement(ContainSubstring("del-draft-br")))) }) - PIt("should clean up proposed git branch when a Proposed package is deleted", func() { + It("should clean up proposed git branch when a Proposed package is deleted", func() { By("creating and proposing a package") pr := newPackageRevision(env.Namespace, env.RepoName, "del-prop-br", "v1", withInit("proposed branch cleanup")) Expect(k8sClient.Create(env.Ctx, pr)).To(Succeed()) diff --git a/test/e2e/suiteutils/gitea_test_utils.go b/test/e2e/suiteutils/gitea_test_utils.go index d78981d21..02275ace9 100644 --- a/test/e2e/suiteutils/gitea_test_utils.go +++ b/test/e2e/suiteutils/gitea_test_utils.go @@ -16,13 +16,19 @@ package suiteutils import ( "context" + "encoding/base64" + "encoding/json" + "fmt" "net/http" "os" "strings" "testing" "time" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -37,6 +43,17 @@ const ( defaultGiteaLBIP = "172.18.255.200" ) +func (t *TestSuite) GetGiteaApiURL() string { + if t.GiteaUrl == GiteaClusterURL { + return "http://localhost:3000" + } + return t.GiteaUrl +} + +func DraftGitBranchName(packageName, workspaceName string) string { + return fmt.Sprintf("drafts/%s/%s", packageName, workspaceName) +} + // getGiteaLBIP returns the Gitea LoadBalancer IP, preferring the GITEA_LB_IP env var. // If not set, it polls the gitea-lb Service until the LoadBalancer IP is allocated // (with a timeout). Falls back to the hardcoded default only if discovery times out. @@ -126,3 +143,287 @@ func RecreateGiteaRepo(t *testing.T, repoName string) { func (t *TestSuite) RecreateGiteaTestRepo() { RecreateGiteaRepo(t.T(), PorchTestRepoName) } + +func (t *TestSuite) CreateGiteaRepo(repoName string) string { + t.T().Helper() + repoURL := t.CreateGiteaRepoNoCleanup(repoName) + t.Cleanup(func() { + t.DeleteGiteaRepo(repoName) + }) + return repoURL +} + +func (t *TestSuite) CreateGiteaRepoNoCleanup(repoName string) string { + t.T().Helper() + + body := fmt.Sprintf(`{"name":%q,"auto_init":true,"readme":"Default"}`, repoName) + req, err := http.NewRequest("POST", t.GetGiteaApiURL()+"/api/v1/user/repos", strings.NewReader(body)) + if err != nil { + t.Fatalf("CreateGiteaRepoNoCleanup: failed to build request: %v", err) + } + + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("CreateGiteaRepoNoCleanup: request failed for %q: %v", repoName, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("CreateGiteaRepoNoCleanup: unexpected status %d creating repo %q", resp.StatusCode, repoName) + } + t.Logf("CreateGiteaRepoNoCleanup: created repo %q", repoName) + + return t.getGiteaURL() + repoName + ".git" +} + +// DeleteGiteaRepo deletes a Gitea repository owned by t.GiteaUser. +func (t *TestSuite) DeleteGiteaRepo(repoName string) { + t.T().Helper() + + req, _ := http.NewRequest("DELETE", + t.GetGiteaApiURL()+"/api/v1/repos/"+t.GiteaUser+"/"+repoName, nil) + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Logf("DeleteGiteaRepo: request failed for %q: %v", repoName, err) + return + } + defer resp.Body.Close() + + t.Logf("DeleteGiteaRepo: deleted repo %q (status %d)", repoName, resp.StatusCode) +} + +// BuildGitRepoObject returns a new configapi.Repository pointing at repoURL using the given secret. +// This is useful when managing two registrations of the same repo within a single test. +func (t *TestSuite) BuildGitRepoObject(repoName, repoURL, secretName string) client.Object { + return &configapi.Repository{ + TypeMeta: metav1.TypeMeta{ + Kind: configapi.TypeRepository.Kind, + APIVersion: configapi.GroupVersion.Identifier(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: repoName, + Namespace: t.Namespace, + }, + Spec: configapi.RepositorySpec{ + Description: "Porch Test Repository", + Type: configapi.RepositoryTypeGit, + Git: &configapi.GitRepository{ + Repo: repoURL, + Branch: "main", + SecretRef: configapi.SecretRef{ + Name: secretName, + }, + }, + }, + } +} + +// GiteaCommitFileToBranch creates a new file on the given branch via the Gitea Contents API. +func (t *TestSuite) GiteaCommitFileToBranch(repoName, branchName, filePath, fileContent, commitMsg string) { + t.T().Helper() + + encodedContent := base64.StdEncoding.EncodeToString([]byte(fileContent)) + body := fmt.Sprintf(`{"message":%q,"content":%q,"branch":%q}`, commitMsg, encodedContent, branchName) + url := fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", + t.GetGiteaApiURL(), t.GiteaUser, repoName, filePath) + req, err := http.NewRequest("POST", url, strings.NewReader(body)) + if err != nil { + t.Fatalf("GiteaCommitFileToBranch: failed to build request: %v", err) + } + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("GiteaCommitFileToBranch: request failed: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("GiteaCommitFileToBranch: unexpected status %d committing %q to branch %q in repo %q", + resp.StatusCode, filePath, branchName, repoName) + } + t.Logf("GiteaCommitFileToBranch: committed %q to branch %q in repo %q", filePath, branchName, repoName) +} + +// GiteaGetBranchLatestCommitSHA returns the latest commit SHA on the given branch. +func (t *TestSuite) GiteaGetBranchLatestCommitSHA(repoName, branchName string) string { + t.T().Helper() + + url := fmt.Sprintf("%s/api/v1/repos/%s/%s/branches/%s", + t.GetGiteaApiURL(), t.GiteaUser, repoName, branchName) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + t.Fatalf("GiteaGetBranchLatestCommitSHA: failed to build request: %v", err) + } + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("GiteaGetBranchLatestCommitSHA: request failed: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("GiteaGetBranchLatestCommitSHA: unexpected status %d for branch %q in repo %q", + resp.StatusCode, branchName, repoName) + } + + var info struct { + Commit struct { + ID string `json:"id"` + } `json:"commit"` + } + if err := json.NewDecoder(resp.Body).Decode(&info); err != nil { + t.Fatalf("GiteaGetBranchLatestCommitSHA: failed to decode response: %v", err) + } + return info.Commit.ID +} + +// WaitUntilGiteaBranchExists polls until the named branch appears in the Gitea repository. +func (t *TestSuite) WaitUntilGiteaBranchExists(repoName, branchName string, timeout time.Duration) { + t.T().Helper() + + err := wait.PollUntilContextTimeout(context.Background(), time.Second, timeout, true, func(ctx context.Context) (bool, error) { + return t.GiteaBranchExists(repoName, branchName), nil + }) + if err != nil { + t.Fatalf("WaitUntilGiteaBranchExists: branch %q in repo %q did not appear within %v", + branchName, repoName, timeout) + } +} + +// GiteaBranchExists returns true when the named branch exists in the given Gitea +func (t *TestSuite) GiteaBranchExists(repoName, branchName string) bool { + t.T().Helper() + + url := fmt.Sprintf("%s/api/v1/repos/%s/%s/branches/%s", + t.GetGiteaApiURL(), t.GiteaUser, repoName, branchName) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + t.Logf("GiteaBranchExists: failed to build request: %v", err) + return false + } + + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Logf("GiteaBranchExists: request failed: %v", err) + return false + } + defer resp.Body.Close() + + return resp.StatusCode == http.StatusOK +} + +// WaitUntilGiteaBranchHasNewCommit polls until the branch's latest commit SHA differs from +// oldCommitSHA and returns the new SHA. +func (t *TestSuite) WaitUntilGiteaBranchHasNewCommit(repoName, branchName, oldCommitSHA string, timeout time.Duration) string { + t.T().Helper() + + var newSHA string + err := wait.PollUntilContextTimeout(context.Background(), time.Second, timeout, true, func(ctx context.Context) (bool, error) { + url := fmt.Sprintf("%s/api/v1/repos/%s/%s/branches/%s", + t.GetGiteaApiURL(), t.GiteaUser, repoName, branchName) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return false, nil + } + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + resp, err := http.DefaultClient.Do(req) + if err != nil { + return false, nil + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return false, nil + } + var info struct { + Commit struct { + ID string `json:"id"` + } `json:"commit"` + } + if err := json.NewDecoder(resp.Body).Decode(&info); err != nil { + return false, nil + } + if info.Commit.ID != "" && info.Commit.ID != oldCommitSHA { + newSHA = info.Commit.ID + return true, nil + } + return false, nil + }) + if err != nil { + t.Fatalf("WaitUntilGiteaBranchHasNewCommit: branch %q in repo %q did not get a new commit (was %q) within %v", + branchName, repoName, oldCommitSHA, timeout) + } + return newSHA +} + +func (t *TestSuite) GiteaRepoTagCount(repoName string) int { + t.T().Helper() + + url := fmt.Sprintf("%s/api/v1/repos/%s/%s/tags", t.GetGiteaApiURL(), t.GiteaUser, repoName) + req, err := http.NewRequest("GET", url, nil) + if err != nil { + t.Logf("GiteaRepoTagCount: failed to build request: %v", err) + return -1 + } + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Logf("GiteaRepoTagCount: request failed: %v", err) + return -1 + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Logf("GiteaRepoTagCount: unexpected status %d for repo %q", resp.StatusCode, repoName) + return -1 + } + + var tags []struct { + Name string `json:"name"` + } + if err := json.NewDecoder(resp.Body).Decode(&tags); err != nil { + t.Logf("GiteaRepoTagCount: failed to decode response: %v", err) + return -1 + } + + return len(tags) +} + +// SetGiteaRepoArchived archives or un-archives a Gitea repository owned by +func (t *TestSuite) SetGiteaRepoArchived(repoName string, archived bool) { + t.T().Helper() + + archivedStr := "false" + if archived { + archivedStr = "true" + } + + body := fmt.Sprintf(`{"archived":%s}`, archivedStr) + req, _ := http.NewRequest("PATCH", + t.GetGiteaApiURL()+"/api/v1/repos/"+t.GiteaUser+"/"+repoName, + strings.NewReader(body)) + req.SetBasicAuth(t.GiteaUser, t.GiteaPassword) + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("SetGiteaRepoArchived: request failed for repo %q: %v", repoName, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("SetGiteaRepoArchived: unexpected status %d for repo %q archived=%v", + resp.StatusCode, repoName, archived) + } + + t.Logf("SetGiteaRepoArchived: repo %q archived=%v", repoName, archived) +} diff --git a/test/e2e/suiteutils/suite.go b/test/e2e/suiteutils/suite.go index 3311fa3b3..7720a0219 100644 --- a/test/e2e/suiteutils/suite.go +++ b/test/e2e/suiteutils/suite.go @@ -50,8 +50,13 @@ import ( const ( // TODO: accept a flag? - PorchTestConfigFile = "porch-test-config.yaml" - updateGoldenFiles = "UPDATE_GOLDEN_FILES" + PorchTestConfigFile = "porch-test-config.yaml" + updateGoldenFiles = "UPDATE_GOLDEN_FILES" + GiteaUserEnv = "GITEA_USER" + GiteaPasswordEnv = "GITEA_PASS" + GiteaClusterUrlEnv = "GITEA_HOST" + defaultGiteaUser = "porch" + defaultGiteaPassword = "secret" ) type GitConfig struct { @@ -79,6 +84,10 @@ type TestSuite struct { // Strongly-typed client handy for reading e.g. pod logs KubeClient kubernetes.Interface + GiteaUser string + GiteaPassword string + GiteaUrl string + Namespace string // K8s namespace for this test run TestRunnerIsLocal bool // Tests running against local dev porch porchServerInCluster *bool // Cached result of IsPorchServerInCluster check @@ -141,6 +150,24 @@ func (t *TestSuite) Initialize() { t.Namespace = namespace + t.GiteaUser = defaultGiteaUser + giteaUser := os.Getenv(GiteaUserEnv) + if giteaUser != "" { + t.GiteaUser = giteaUser + } + + t.GiteaPassword = defaultGiteaPassword + giteaPassword := os.Getenv(GiteaPasswordEnv) + if giteaPassword != "" { + t.GiteaPassword = giteaPassword + } + + t.GiteaUrl = GiteaClusterURL + giteaUrl := os.Getenv(GiteaClusterUrlEnv) + if giteaUrl != "" { + t.GiteaUrl = "http://" + giteaUrl + } + t.checkIfUsingDBCache() c := t.Client diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 14f139bfe..e7ade2272 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -764,6 +764,123 @@ func (t *TestSuite) GetPackageRevision(repo string, pkgName string, revision int return &prList.Items[0] } +type PackageRevisionFilter struct { + Revision int + Workspace string +} + +func (t *TestSuite) GetPackageRevisionWithWS(repoName, packageName, workspace string) *porchapi.PackageRevision { + t.T().Helper() + return t.GetPackageRevisionWithFilter(repoName, packageName, PackageRevisionFilter{Workspace: workspace}) +} + +func (t *TestSuite) GetPackageRevisionWithFilter(repo, pkgName string, filter PackageRevisionFilter) *porchapi.PackageRevision { + t.T().Helper() + var prList porchapi.PackageRevisionList + fieldSet := fields.Set{ + "spec.repository": repo, + "spec.packageName": pkgName, + } + if filter.Revision != 0 { + fieldSet["spec.revision"] = strconv.Itoa(filter.Revision) + } + if filter.Workspace != "" { + fieldSet["spec.workspaceName"] = filter.Workspace + } + t.ListF(&prList, client.MatchingFields(fieldSet), client.InNamespace(t.Namespace)) + + if len(prList.Items) == 0 { + t.Fatalf("PackageRevision object wasn't found for package revision %v/%v with filter %+v", repo, pkgName, filter) + } + if len(prList.Items) > 1 { + t.Fatalf("Multiple PackageRevision objects were found for package revision %v/%v with filter %+v", repo, pkgName, filter) + } + return &prList.Items[0] +} + +// RequestRepoSync schedules a one-time repository sync via spec.sync.runOnceAt. +// Unlike TriggerRepoSync, it does not wait for the sync to complete. +func (t *TestSuite) RequestRepoSync(repoName string) time.Time { + t.T().Helper() + repoKey := client.ObjectKey{Namespace: t.Namespace, Name: repoName} + + var repo configapi.Repository + t.GetF(repoKey, &repo) + + if repo.Spec.Sync == nil { + repo.Spec.Sync = &configapi.RepositorySync{} + } + // Schedule runOnceAt slightly in the past so the controller's isOneTimeSyncDue + // check triggers a full sync on the next reconcile without an extra delay. + runOnceAt := metav1.NewTime(time.Now().Add(-1 * time.Second)) + repo.Spec.Sync.RunOnceAt = new(runOnceAt) + t.UpdateF(&repo) + + t.Logf("RequestRepoSync: set runOnceAt for repo %s", repoName) + return runOnceAt.Time +} + +func (t *TestSuite) TriggerRepoSync(repoName string, timeout time.Duration) { + t.T().Helper() + repoKey := client.ObjectKey{Namespace: t.Namespace, Name: repoName} + + var repo configapi.Repository + t.GetF(repoKey, &repo) + + baselineLastSync := time.Time{} + if repo.Status.LastFullSyncTime != nil { + baselineLastSync = repo.Status.LastFullSyncTime.Time + } + + runOnceAt := t.RequestRepoSync(repoName) + + t.Logf("TriggerRepoSync: waiting for sync to complete for repo %s", repoName) + t.WaitForNextRepoSync(repoName, timeout, baselineLastSync, runOnceAt) +} + +func (t *TestSuite) WaitForNextRepoSync(repoName string, timeout time.Duration, baselineLastSync, triggeredRunOnceAt time.Time) { + t.T().Helper() + repoKey := client.ObjectKey{Namespace: t.Namespace, Name: repoName} + + t.Logf("WaitForNextRepoSync: waiting for repo %s full sync after LastFullSyncTime %v (timeout %v)", + repoName, baselineLastSync, timeout) + waitErr := wait.PollUntilContextTimeout(t.GetContext(), 1*time.Second, timeout, false, func(ctx context.Context) (bool, error) { + var latest configapi.Repository + if err := t.Reader.Get(ctx, repoKey, &latest); err != nil { + return false, err + } + + ready := false + for _, cond := range latest.Status.Conditions { + if cond.Type == configapi.RepositoryReady && cond.Status == metav1.ConditionTrue { + ready = true + break + } + } + if !ready { + return false, nil + } + + if latest.Status.LastFullSyncTime != nil && latest.Status.LastFullSyncTime.Time.After(baselineLastSync) { + t.Logf("WaitForNextRepoSync: repo %s sync completed (LastFullSyncTime=%s)", + repoName, latest.Status.LastFullSyncTime.Time.Format(time.RFC3339)) + return true, nil + } + + if latest.Status.ObservedRunOnceAt != nil && latest.Status.ObservedRunOnceAt.Time.Equal(triggeredRunOnceAt) { + t.Logf("WaitForNextRepoSync: repo %s runOnceAt sync completed (ObservedRunOnceAt=%s)", + repoName, latest.Status.ObservedRunOnceAt.Time.Format(time.RFC3339)) + return true, nil + } + + return false, nil + }) + + if waitErr != nil { + t.Errorf("WaitForNextRepoSync: repo %s sync did not complete within %v: %v", repoName, timeout, waitErr) + } +} + func (t *TestSuite) RetriggerBackgroundJobForRepo(repoName string) { repoKey := client.ObjectKey{ Namespace: t.Namespace,