From 1bbb4fa6044e9cf548f2a581d9a690636fb8a621 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Wed, 5 Aug 2026 13:41:45 +0200 Subject: [PATCH 01/13] Preliminary draft pushing refactor Signed-off-by: Rendre Greyling --- api/sql/porch-db-1.6.0-1.6.4.sql | 20 + api/sql/porch-db-1.6.4-1.6.0.sql | 20 + api/sql/porch-db.sql | 3 + .../porch/3-porch-postgres-bundle.yaml | 3 + pkg/cache/dbcache/dbpackagerevision.go | 95 +-- pkg/cache/dbcache/dbpackagerevisionsql.go | 104 ++- pkg/cache/dbcache/dbpushtogit.go | 268 ++++++++ .../dbcache/dbpushtogit_test.go} | 2 +- pkg/cache/dbcache/dbrepository.go | 142 +--- pkg/cache/dbcache/dbreposync.go | 239 ++++++- pkg/cache/dbcache/dbreposync_test.go | 612 ++++++++++++++++++ pkg/cache/dbcache/util.go | 105 +++ pkg/cache/dbcache/util_test.go | 241 +++++++ pkg/engine/pushpr.go | 174 ----- pkg/externalrepo/git/git.go | 33 +- pkg/externalrepo/git/package.go | 25 +- pkg/externalrepo/git/package_tree.go | 20 +- pkg/repository/repository.go | 6 + test/e2e/api/db_git_sync_test.go | 469 ++++++++++++++ test/e2e/suiteutils/gitea_test_utils.go | 305 +++++++++ test/e2e/suiteutils/suite.go | 31 +- test/e2e/suiteutils/suite_utils.go | 91 +++ 22 files changed, 2610 insertions(+), 398 deletions(-) create mode 100644 api/sql/porch-db-1.6.0-1.6.4.sql create mode 100644 api/sql/porch-db-1.6.4-1.6.0.sql create mode 100644 pkg/cache/dbcache/dbpushtogit.go rename pkg/{engine/pushpr_test.go => cache/dbcache/dbpushtogit_test.go} (99%) delete mode 100644 pkg/engine/pushpr.go create mode 100644 test/e2e/api/db_git_sync_test.go 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..4eddd3000 --- /dev/null +++ b/api/sql/porch-db-1.6.0-1.6.4.sql @@ -0,0 +1,20 @@ +/* +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_commit TEXT, + ADD COLUMN IF NOT EXISTS last_pushed_commit_timestamp TIMESTAMP, + ADD COLUMN IF NOT EXISTS last_pushed_db_updated TIMESTAMP; \ No newline at end of file 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..b8bac1796 --- /dev/null +++ b/api/sql/porch-db-1.6.4-1.6.0.sql @@ -0,0 +1,20 @@ +/* +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_commit, + DROP COLUMN IF EXISTS last_pushed_commit_timestamp, + DROP COLUMN IF EXISTS last_pushed_db_updated; \ No newline at end of file diff --git a/api/sql/porch-db.sql b/api/sql/porch-db.sql index e19d9ab63..832285676 100644 --- a/api/sql/porch-db.sql +++ b/api/sql/porch-db.sql @@ -88,6 +88,9 @@ 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_commit TEXT, + last_pushed_commit_timestamp TIMESTAMP, + 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..47a703977 100644 --- a/deployments/porch/3-porch-postgres-bundle.yaml +++ b/deployments/porch/3-porch-postgres-bundle.yaml @@ -356,6 +356,9 @@ data: kptfile_status TEXT NOT NULL DEFAULT '{}', resources_size BIGINT NOT NULL DEFAULT 0, upstream_ref_name TEXT NOT NULL DEFAULT '', + last_pushed_commit TEXT, + last_pushed_commit_timestamp TIMESTAMP, + 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/pkg/cache/dbcache/dbpackagerevision.go b/pkg/cache/dbcache/dbpackagerevision.go index 5868a88f1..5d09c66be 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" @@ -92,11 +91,18 @@ type dbPackageRevision struct { kptfileStatus kptfileStatus resourcesSizeBytes int64 - // gitPRDraft maintains the draft in the external git repository during editing (when pushDraftsToGit is true) - gitPRDraft repository.PackageRevisionDraft + // lastPushedCommit is the git commit hash of the last successful push of this revision to git. + // A nil value means the revision has never been (successfully) pushed to git. + lastPushedCommit *string - // gitPR is the closed package revision in git (when pushDraftsToGit is true) - gitPR repository.PackageRevision + // lastPushedCommitTimestamp is the timestamp associated with the last commit pushed to git. + // It is used by conflict resolution to reason about the git side of the last push. + lastPushedCommitTimestamp *time.Time + + // lastPushedDbUpdated is the value of the DB `updated` column at the time of the last successful + // push to git. It is used to detect whether the revision content has changed since it was last + // pushed, and by conflict resolution to reason about the DB side of the last push. + lastPushedDbUpdated *time.Time } // ensureRepo resolves the repository from the cache if pr.repo is nil. @@ -158,8 +164,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 +209,10 @@ func (pr *dbPackageRevision) UpdateLifecycle(ctx context.Context, newLifecycle p _, span := tracer.Start(ctx, "dbPackageRevision::UpdateLifecycle", trace.WithAttributes()) defer span.End() + pkgMutex := getOrInsertPkgLock(pr.pkgRevKey.PkgKey) + pkgMutex.Lock() + defer pkgMutex.Unlock() + if err := pr.ensureRepo(); err != nil { return fmt.Errorf("cannot update lifecycle for package revision %s: %w", pr.KubeObjectName(), err) } @@ -214,11 +225,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 +237,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 +406,18 @@ func (pr *dbPackageRevision) SetMeta(ctx context.Context, meta metav1.ObjectMeta _, span := tracer.Start(ctx, "dbPackageRevision::SetMeta", trace.WithAttributes()) defer span.End() + pkgMutex := getOrInsertPkgLock(pr.pkgRevKey.PkgKey) + pkgMutex.Lock() + defer pkgMutex.Unlock() + 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 +491,18 @@ func (pr *dbPackageRevision) copyToThis(otherPr *dbPackageRevision) { pr.tasks = otherPr.tasks pr.resources = otherPr.resources pr.resourcesSizeBytes = otherPr.resourcesSizeBytes + pr.lastPushedCommit = otherPr.lastPushedCommit + pr.lastPushedCommitTimestamp = otherPr.lastPushedCommitTimestamp + pr.lastPushedDbUpdated = otherPr.lastPushedDbUpdated +} + +func preservePushMarkersIfUnset(pr, existing *dbPackageRevision) { + if pr.lastPushedCommit != nil || existing.lastPushedCommit == nil { + return + } + pr.lastPushedCommit = existing.lastPushedCommit + pr.lastPushedCommitTimestamp = existing.lastPushedCommitTimestamp + pr.lastPushedDbUpdated = existing.lastPushedDbUpdated } func (pr *dbPackageRevision) UpdateResources(ctx context.Context, new *porchapi.PackageRevisionResources, change *porchapi.Task) error { @@ -492,18 +513,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 +532,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 +547,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, commitTimestamp, err := PushPublishedPackageRevision(ctx, pr.repo.externalRepo, pr, pr.repo.pushDraftsToGit, pr.lastPushedCommit != nil) if err != nil { klog.Warningf("push of package revision %+v to external repo failed, %q", pr.Key(), err) pr.pkgRevKey.Revision = 0 @@ -562,6 +556,13 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap } pr.extPRID = pushedPRExtID + if pushedPRExtID.Git != nil && pushedPRExtID.Git.Commit != "" { + pr.lastPushedCommit = new(pushedPRExtID.Git.Commit) + if commitTimestamp.IsZero() { + commitTimestamp = time.Now() + } + pr.lastPushedCommitTimestamp = &commitTimestamp + } 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/dbpackagerevisionsql.go b/pkg/cache/dbcache/dbpackagerevisionsql.go index 9610281b3..3acb881eb 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" @@ -54,6 +55,9 @@ func pkgRevReadFromDB(ctx context.Context, prk repository.PackageRevisionKey, re package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size + package_revisions.last_pushed_commit, + package_revisions.last_pushed_commit_timestamp, + 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 @@ -127,6 +131,9 @@ func pkgRevListPRsFromDB(ctx context.Context, filter repository.ListPackageRevis package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size + package_revisions.last_pushed_commit, + package_revisions.last_pushed_commit_timestamp, + 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 @@ -177,6 +184,9 @@ func pkgRevReadPRsFromDB(ctx context.Context, pk repository.PackageKey) ([]*dbPa package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size + package_revisions.last_pushed_commit, + package_revisions.last_pushed_commit_timestamp, + 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 @@ -228,6 +238,9 @@ func pkgRevReadLatestPRFromDB(ctx context.Context, pk repository.PackageKey) (*d package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size + package_revisions.last_pushed_commit, + package_revisions.last_pushed_commit_timestamp, + 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 +308,8 @@ 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 lastPushedCommit sql.NullString + var lastPushedCommitTimestamp, lastPushedDbUpdated sql.NullTime err := rows.Scan( &pkgRev.pkgRevKey.PkgKey.RepoKey.Namespace, @@ -315,13 +330,29 @@ func pkgRevScanRowsFromDB(ctx context.Context, rows *sql.Rows) ([]*dbPackageRevi &pkgRev.latest, &tasks, &kptfileStatusJSON, - &pkgRev.resourcesSizeBytes) + &pkgRev.resourcesSizeBytes, + &lastPushedCommit, + &lastPushedCommitTimestamp, + &lastPushedDbUpdated) if err != nil { klog.Warningf("pkgRevScanRowsFromDB: scanning rows failed: %q", err) return nil, err } + if lastPushedCommit.Valid { + commit := lastPushedCommit.String + pkgRev.lastPushedCommit = &commit + } + if lastPushedCommitTimestamp.Valid { + commitTimestamp := lastPushedCommitTimestamp.Time + pkgRev.lastPushedCommitTimestamp = &commitTimestamp + } + 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 +382,21 @@ 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_commit, last_pushed_commit_timestamp, last_pushed_db_updated) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17) ` klog.V(6).Infof("pkgRevWriteToDB: running query %q on package revision %+v", sqlStatement, pr) + + lastPushedCommit := lastPushedCommitAsNullString(pr) + lastPushedCommitTimestamp := lastPushedCommitTimestampAsNullTime(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), lastPushedCommit, lastPushedCommitTimestamp, 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 +419,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_commit=$15, last_pushed_commit_timestamp=$16, last_pushed_db_updated=$17 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_commit, last_pushed_commit_timestamp, 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, $16, $17 ) ON CONFLICT (k8s_name_space, k8s_name) DO UPDATE SET @@ -406,16 +442,24 @@ 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_commit = EXCLUDED.last_pushed_commit, + last_pushed_commit_timestamp = EXCLUDED.last_pushed_commit_timestamp, + last_pushed_db_updated = EXCLUDED.last_pushed_db_updated; ` } klog.V(6).Infof("pkgRevUpdateDB: running query %q on package revision %+v", sqlStatement, pr) + + lastPushedCommit := lastPushedCommitAsNullString(pr) + lastPushedCommitTimestamp := lastPushedCommitTimestampAsNullTime(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), lastPushedCommit, lastPushedCommitTimestamp, lastPushedDbUpdated) if err == nil { if rowsAffected, _ := result.RowsAffected(); rowsAffected == 1 { @@ -703,3 +747,45 @@ func backfillUpstreamRefName(ctx context.Context) error { } return nil } + +func lastPushedCommitAsNullString(pr *dbPackageRevision) sql.NullString { + if pr.lastPushedCommit == nil { + return sql.NullString{} + } + return sql.NullString{Valid: true, String: *pr.lastPushedCommit} +} + +func lastPushedCommitTimestampAsNullTime(pr *dbPackageRevision) sql.NullTime { + if pr.lastPushedCommitTimestamp == nil { + return sql.NullTime{} + } + return sql.NullTime{Valid: true, Time: *pr.lastPushedCommitTimestamp} +} + +func lastPushedDbUpdatedAsNullTime(pr *dbPackageRevision) sql.NullTime { + if pr.lastPushedDbUpdated == nil { + return sql.NullTime{} + } + return sql.NullTime{Valid: true, Time: *pr.lastPushedDbUpdated} +} + +// pkgRevSetLastPushedInDB records the last successfully pushed git commit (and its timestamp) for a +// package revision without touching the `updated`/`updatedby` columns. +func pkgRevSetLastPushedInDB(ctx context.Context, prk repository.PackageRevisionKey, commit string, commitTimestamp time.Time, expectedUpdated time.Time) (bool, error) { + _, span := tracer.Start(ctx, "dbpackagerevisionsql::pkgRevSetLastPushedInDB", trace.WithAttributes()) + defer span.End() + + sqlStatement := ` + UPDATE package_revisions SET last_pushed_commit=$3, last_pushed_commit_timestamp=$4, last_pushed_db_updated=$5 + WHERE k8s_name_space=$1 AND k8s_name=$2 AND updated=$5 + ` + + result, err := GetDB().db.Exec(ctx, sqlStatement, prk.K8SNS(), prk.K8SName(), commit, commitTimestamp, 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..82ef6c6a8 --- /dev/null +++ b/pkg/cache/dbcache/dbpushtogit.go @@ -0,0 +1,268 @@ +// 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 dbcache + +import ( + "context" + "database/sql" + "fmt" + "time" + + 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, time.Time, 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{}, time.Time{}, 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{}, time.Time{}, 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{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get package revision resources:", pr.Key(), repo.Key()) + } + + commitTask := &porchapi.Task{Type: porchapi.TaskTypePush} + if len(apiPr.Spec.Tasks) > 0 { + commitTask = &apiPr.Spec.Tasks[0] + } + + 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{}, time.Time{}, 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, commitTask); err != nil { + return kptfilev1.Locator{}, time.Time{}, 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{}, time.Time{}, 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, commitTask); err != nil { + return kptfilev1.Locator{}, time.Time{}, 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{}, time.Time{}, 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{}, time.Time{}, 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{}, time.Time{}, pkgerrors.Wrapf(err, "read of upstream lock for package revision %+v pushed to repository %+v failed", pr.Key(), repo.Key()) + } + + // Capture the actual git commit timestamp when the backend exposes it so callers can persist + // the git-generated timestamp rather than a locally generated one. + var commitTimestamp time.Time + if ctg, ok := pushedPR.(repository.CommitTimeGetter); ok { + commitTimestamp = ctg.CommitTimestamp() + } + + return pushedPRUpstreamLock, commitTimestamp, nil +} + +func PushDraftPackageRevision(ctx context.Context, repoKey repository.RepositoryKey, pr *dbPackageRevision) { + prKey := pr.Key() + klog.Infof("PushDraftPackageRevision: repo %+v started for %+v", repoKey, prKey) + + pkgMutex := getOrInsertPkgLock(prKey.PKey()) + pkgMutex.Lock() + defer func() { + pkgMutex.Unlock() + deletePkgLock(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, _, 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)); 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 + } + + commit := "" + if pushedLock.Git != nil { + commit = pushedLock.Git.Commit + } + + commitTimestamp := time.Now() + if ctg, ok := pushedGitPR.(repository.CommitTimeGetter); ok { + if gitTime := ctg.CommitTimestamp(); !gitTime.IsZero() { + commitTimestamp = gitTime + } + } + + recorded, err := pkgRevSetLastPushedInDB(ctx, prKey, commit, commitTimestamp, updatedBeforePush) + if err != nil { + klog.Warningf("PushDraftPackageRevision: repo %+v: failed to record last_pushed_commit 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 last_pushed_commit — next sync will retry with fresh data", + repoKey, prKey, updatedBeforePush) + return + } + + 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 99% rename from pkg/engine/pushpr_test.go rename to pkg/cache/dbcache/dbpushtogit_test.go index fa7128be8..98f6dc7a9 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" diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 147402859..0cb631b3c 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{ @@ -267,21 +232,6 @@ func (r *dbRepository) CreatePackageRevisionDraft(ctx context.Context, newPR *po }, } - 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,13 @@ func (r *dbRepository) DeletePackageRevision(ctx context.Context, pr2Delete repo Package: pr2Delete.Key().PKey().Package, } + pkgMutex := getOrInsertPkgLock(pk) + pkgMutex.Lock() + defer func() { + pkgMutex.Unlock() + deletePkgLock(pk) + }() + foundPkg, err := pkgReadFromDB(ctx, pk) if err != nil { return err @@ -347,10 +304,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,6 +343,10 @@ func (r *dbRepository) UpdatePackageRevision(ctx context.Context, updatePR repos updatePkgRev.repo = r } + mutex := getOrInsertPkgLock(updatePkgRev.Key().PKey()) + mutex.Lock() + defer mutex.Unlock() + if err := updatePkgRev.UpdatePackageRevision(ctx); err != nil { return nil, err } @@ -397,25 +354,6 @@ func (r *dbRepository) UpdatePackageRevision(ctx context.Context, updatePR repos 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 +404,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 +416,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 +425,10 @@ func (r *dbRepository) savePackageRevisionDraft(ctx context.Context, prd reposit d := prd.(*dbPackageRevision) + repoMutex := getOrInsertRepoLock(r.repoKey) + repoMutex.Lock() + defer repoMutex.Unlock() + return r.savePackageRevision(ctx, d, d.resourcesDirty) } @@ -520,6 +436,10 @@ func (r *dbRepository) savePackageRevision(ctx context.Context, d *dbPackageRevi _, span := tracer.Start(ctx, "dbRepository::savePackageRevision", trace.WithAttributes()) defer span.End() + pkgMutex := getOrInsertPkgLock(d.Key().PKey()) + pkgMutex.Lock() + defer pkgMutex.Unlock() + dbPkg, err := pkgReadFromDB(ctx, d.Key().PKey()) if err != nil { if err != sql.ErrNoRows { @@ -556,9 +476,15 @@ func (r *dbRepository) Refresh(ctx context.Context) error { _, span := tracer.Start(ctx, "dbRepository::Refresh", trace.WithAttributes()) defer span.End() + repoMutex := getOrInsertRepoLock(r.Key()) + repoMutex.Lock() + defer repoMutex.Unlock() + + r.repositorySync.mutex.Lock() if err := r.externalRepo.Refresh(ctx); err != nil { 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/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index ec8ee1827..132844723 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" @@ -28,6 +29,7 @@ import ( "github.com/kptdev/porch/pkg/repository" pkgerrors "github.com/pkg/errors" "go.opentelemetry.io/otel/trace" + "k8s.io/apimachinery/pkg/watch" "k8s.io/klog/v2" ) @@ -98,7 +100,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 +108,10 @@ func (s *repositorySync) sync(ctx context.Context) (repositorySyncStats, error) return repositorySyncStats{}, err } + if s.repo.pushDraftsToGit { + s.reconcileBothPRs(ctx, cachedPrMap, externalPrMap, inBoth) + } + return repositorySyncStats{ cachedOnly: len(inCachedOnly), externalOnly: len(inExternalOnly), @@ -198,25 +204,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 +212,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 +244,209 @@ 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) && + dbPkgRev.lastPushedCommit == nil { + 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 (last_pushed_commit is null)", 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() + pkgMutex := getOrInsertPkgLock(pkgKey) + pkgMutex.Lock() + defer func() { + pkgMutex.Unlock() + deletePkgLock(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) && + freshPR.lastPushedCommit == nil { + 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) reconcileBothPRs(ctx context.Context, cachedPrMap, externalPrMap map[repository.PackageRevisionKey]repository.PackageRevision, inBoth []repository.PackageRevisionKey) { + ctx, span := tracer.Start(ctx, "Repository::reconcileBothPRs", 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 } + + extPR := externalPrMap[prKey] + externalCommit, externalCommitTime := externalCommitInfo(ctx, extPR) + + dbChanged := dbContentChangedSincePush(cachedPR) + extChanged := extCommitChangedSincePush(cachedPR, externalCommit, externalCommitTime) + + switch { + case !dbChanged && !extChanged: + // In sync + case dbChanged && !extChanged: + klog.Infof("repositorySync %+v: reconcile %+v: DB changed since last push, pushing to git", s.repo.Key(), prKey) + s.enqueuePush(ctx, cachedPR) + case !dbChanged && extChanged: + klog.Infof("repositorySync %+v: reconcile %+v: incoming git commit %q, pulling into DB", s.repo.Key(), prKey, externalCommit) + if err := s.pullExternalIntoDB(ctx, cachedPR, extPR, externalCommit, externalCommitTime); err != nil { + klog.Warningf("repositorySync %+v: reconcile %+v: failed to pull incoming commit into DB: %v", s.repo.Key(), prKey, err) + } + default: + // Both DB and External Git has changed which may result in a conflict + // TODO decide if conflict resolution is at all necessary - for now DB will always overwrite Git changes if both were changed + s.enqueuePush(ctx, cachedPR) + } + } +} + +func (s *repositorySync) pullExternalIntoDB(ctx context.Context, cachedPR *dbPackageRevision, extPR repository.PackageRevision, externalCommit string, externalCommitTime time.Time) error { + prKey := cachedPR.Key() + + extAPIPR, err := extPR.GetPackageRevision(ctx) + if err != nil { + return pkgerrors.Wrapf(err, "failed to get external package revision %+v", prKey) + } + + extPRResources, err := extPR.GetResources(ctx) + if err != nil { + return pkgerrors.Wrapf(err, "failed to get resources for external package revision %+v", prKey) + } + resources, resourcesSize := s.sanitizeResources(prKey, extPRResources) + + _, extPRUpstreamLock, _ := extPR.GetLock(ctx) + + pkgMutex := getOrInsertPkgLock(prKey.PKey()) + pkgMutex.Lock() + defer func() { + pkgMutex.Unlock() + deletePkgLock(prKey.PKey()) + }() + + cachedPR.meta = extAPIPR.ObjectMeta + cachedPR.spec = &extAPIPR.Spec + cachedPR.lifecycle = extAPIPR.Spec.Lifecycle + + if len(extAPIPR.Spec.Tasks) > 0 || len(cachedPR.tasks) == 0 { + cachedPR.tasks = extAPIPR.Spec.Tasks + } else { + klog.Warningf("repositorySync %+v: pullExternalIntoDB: external package revision %+v has no tasks, keeping %d cached task(s)", + s.repo.Key(), prKey, len(cachedPR.tasks)) } + + cachedPR.resources = resources + cachedPR.extPRID = extPRUpstreamLock + cachedPR.resourcesSizeBytes = resourcesSize + + commit := externalCommit + commitTime := externalCommitTime + cachedPR.lastPushedCommit = &commit + cachedPR.lastPushedCommitTimestamp = &commitTime + + if err := pkgRevUpdateDB(ctx, cachedPR, true); err != nil { + return pkgerrors.Wrapf(err, "failed to update cached package revision %+v from external repo", prKey) + } + + dbUpdated := cachedPR.updated + cachedPR.lastPushedDbUpdated = &dbUpdated + if _, err := pkgRevSetLastPushedInDB(ctx, prKey, commit, commitTime, cachedPR.updated); err != nil { + klog.Warningf("repositorySync %+v: pullExternalIntoDB: failed to record last_pushed markers for %+v: %v", s.repo.Key(), prKey, err) + } + + sent := cachedPR.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Modified, cachedPR) + klog.Infof("DB cache %+v: sent %d notifications for package revision %+v updated from external repo", s.repo.Key(), sent, prKey) + return nil } + +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..e37acd9a7 100644 --- a/pkg/cache/dbcache/dbreposync_test.go +++ b/pkg/cache/dbcache/dbreposync_test.go @@ -1111,3 +1111,615 @@ 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_AllowSyncDeletionFalse verifies that no PRs are deleted +// when allowSyncDeletion is false, even when they are cached-only. +func (t *DbTestSuite) TestHandleInCachedOnly_AllowSyncDeletionFalse() { + 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 complete without error and NOT delete the PR + err = repoSync.handleInCachedOnly(ctx, cachedPrMap, inCachedOnly) + t.Require().NoError(err) + + // PR should still be present since allowSyncDeletion=false + prListAfter, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) + t.Require().NoError(err) + t.Len(prListAfter, 2, "PR should not be deleted when allowSyncDeletion=false") +} + +// TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse verifies that a Draft PR +// with nil lastPushedCommit 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 (lastPushedCommit 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 nil lastPushedCommit) + cachedPRTyped := dbPR.(*dbPackageRevision) + t.Nil(cachedPRTyped.lastPushedCommit, "lastPushedCommit should be nil for a new draft") + + 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") +} + +// TestPullExternalIntoDB verifies that pullExternalIntoDB correctly updates a cached +// Draft PR with data from the external repository. +func (t *DbTestSuite) TestPullExternalIntoDB() { + ctx := t.Context() + externalrepo.ExternalRepoInUnitTestMode = true + + testRepo := t.createTestRepo("pull-ns", "pull-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 a Draft PR in the DB + newPRDef := porchapi.PackageRevision{ + Spec: porchapi.PackageRevisionSpec{ + RepositoryName: "pull-repo", + PackageName: "pull-pkg", + WorkspaceName: "pull-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) + + cachedPR := dbPR.(*dbPackageRevision) + t.Nil(cachedPR.lastPushedCommit, "new draft should have no lastPushedCommit") + + // Construct the external PR with updated content + updatedPRDef := &porchapi.PackageRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pull-pr-updated", + Namespace: "pull-ns", + CreationTimestamp: metav1.Now(), + }, + Spec: porchapi.PackageRevisionSpec{ + RepositoryName: "pull-repo", + PackageName: "pull-pkg", + WorkspaceName: "pull-ws", + Lifecycle: porchapi.PackageRevisionLifecycleDraft, + }, + } + + updatedResources := &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": "apiVersion: kpt.dev/v1\nkind: Kptfile\n", + "updated-config.yaml": "newKey: newValue\n", + }, + }, + } + + fakeExtPR := &fake.FakePackageRevision{ + PrKey: cachedPR.Key(), + PackageRevision: updatedPRDef, + PackageLifecycle: porchapi.PackageRevisionLifecycleDraft, + Resources: updatedResources, + Kptfile: kptfilev1.KptFile{ + Upstream: &kptfilev1.Upstream{}, + UpstreamLock: &kptfilev1.Locator{}, + }, + } + + externalCommit := "abc123deadbeef" + externalCommitTime := time.Now() + + // pullExternalIntoDB should update the cached PR with the external data + err = repoSync.pullExternalIntoDB(ctx, cachedPR, fakeExtPR, externalCommit, externalCommitTime) + t.Require().NoError(err) + + // Verify the cached PR was updated with the external commit info + t.Require().NotNil(cachedPR.lastPushedCommit, "lastPushedCommit should be set after pull") + t.Equal(externalCommit, *cachedPR.lastPushedCommit) + + // Read back from DB to confirm persistence + freshPR, err := pkgRevReadFromDB(ctx, cachedPR.Key(), false) + t.Require().NoError(err) + t.Require().NotNil(freshPR.lastPushedCommit, "lastPushedCommit should be persisted in DB") + t.Equal(externalCommit, *freshPR.lastPushedCommit) +} diff --git a/pkg/cache/dbcache/util.go b/pkg/cache/dbcache/util.go index 2ffbe4477..265b00339 100644 --- a/pkg/cache/dbcache/util.go +++ b/pkg/cache/dbcache/util.go @@ -15,9 +15,14 @@ package dbcache import ( + "context" "encoding/json" "os/user" + "sync" + "time" + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/pkg/repository" "k8s.io/klog/v2" ) @@ -44,3 +49,103 @@ func setValueFromJSON(jsonValue string, value any) { klog.Errorf("unmarshal of json value %v failed, %v ", jsonValue, err) } } + +type lockManager struct { + mu sync.RWMutex + locks map[string]*sync.Mutex +} + +var globalLockManager = &lockManager{ + locks: make(map[string]*sync.Mutex), +} + +func (lm *lockManager) getLock(key string) *sync.Mutex { + lm.mu.RLock() + if m, exists := lm.locks[key]; exists { + lm.mu.RUnlock() + return m + } + lm.mu.RUnlock() + + lm.mu.Lock() + defer lm.mu.Unlock() + if m, exists := lm.locks[key]; exists { + return m + } + + lm.locks[key] = new(sync.Mutex) + return lm.locks[key] +} + +func (lm *lockManager) deleteLock(key string) { + lm.mu.Lock() + defer lm.mu.Unlock() + delete(lm.locks, key) +} + +func getOrInsertRepoLock(repoKey repository.RepositoryKey) *sync.Mutex { + return globalLockManager.getLock(repoKey.String()) +} + +func getOrInsertPkgLock(pkgKey repository.PackageKey) *sync.Mutex { + return globalLockManager.getLock(pkgKey.String()) +} + +func deletePkgLock(pkgKey repository.PackageKey) { + globalLockManager.deleteLock(pkgKey.String()) +} + +func externalCommitInfo(ctx context.Context, extPR repository.PackageRevision) (string, time.Time) { + var commit string + if _, lock, err := extPR.GetLock(ctx); err == nil && lock.Git != nil { + commit = lock.Git.Commit + } + + var commitTime time.Time + if ctg, ok := extPR.(repository.CommitTimeGetter); ok { + commitTime = ctg.CommitTimestamp() + } + + return commit, commitTime +} + +func dbContentChangedSincePush(pr *dbPackageRevision) bool { + if pr.lastPushedDbUpdated == nil { + return true + } + return !pr.updated.Equal(*pr.lastPushedDbUpdated) +} + +func extCommitChangedSincePush(pr *dbPackageRevision, externalCommit string, externalCommitTime time.Time) bool { + if externalCommit == "" || pr.lastPushedCommit == nil { + return false + } + if externalCommit == *pr.lastPushedCommit { + return false + } + if pr.lastPushedCommitTimestamp == nil { + return true + } + return externalCommitTime.After(*pr.lastPushedCommitTimestamp) +} + +func commitTaskForPush(pr *dbPackageRevision) *porchapi.Task { + if pr.lastPushedCommit != nil { + 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 prNeedsPushToGit(pr *dbPackageRevision) bool { + if pr.lastPushedCommit == nil || 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..01c7a7619 100644 --- a/pkg/cache/dbcache/util_test.go +++ b/pkg/cache/dbcache/util_test.go @@ -15,7 +15,17 @@ package dbcache import ( + "context" + "errors" + "os/user" + "sync" "time" + + kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + "github.com/kptdev/porch/pkg/repository" + mockrepo "github.com/kptdev/porch/test/mockery/mocks/porch/pkg/repository" + "github.com/stretchr/testify/mock" ) func (t *DbTestSuite) TestUtil() { @@ -27,3 +37,234 @@ 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) TestLockManagerGetLock() { + lm := &lockManager{locks: make(map[string]*sync.Mutex)} + + lock1 := lm.getLock("key1") + t.NotNil(lock1) + + lock1Again := lm.getLock("key1") + t.Same(lock1, lock1Again) + + lock2 := lm.getLock("key2") + t.NotNil(lock2) + t.NotSame(lock1, lock2) +} + +func (t *DbTestSuite) TestLockManagerDeleteLock() { + lm := &lockManager{locks: make(map[string]*sync.Mutex)} + + lock := lm.getLock("mykey") + t.NotNil(lock) + t.Len(lm.locks, 1) + + lm.deleteLock("mykey") + t.Len(lm.locks, 0) + + lm.deleteLock("non-existent") +} + +func (t *DbTestSuite) TestGetOrInsertRepoLock() { + repoKey := repository.RepositoryKey{Namespace: "ns", Name: "repo"} + lock := getOrInsertRepoLock(repoKey) + t.NotNil(lock) + + lock2 := getOrInsertRepoLock(repoKey) + t.Same(lock, lock2) +} + +func (t *DbTestSuite) TestGetOrInsertPkgLockAndDeletePkgLock() { + pkgKey := repository.PackageKey{ + RepoKey: repository.RepositoryKey{Namespace: "ns", Name: "repo"}, + Package: "my-pkg", + } + + lock := getOrInsertPkgLock(pkgKey) + t.NotNil(lock) + + lock2 := getOrInsertPkgLock(pkgKey) + t.Same(lock, lock2) + + deletePkgLock(pkgKey) + lock3 := getOrInsertPkgLock(pkgKey) + t.NotNil(lock3) + + deletePkgLock(pkgKey) +} + +func (t *DbTestSuite) TestExternalCommitInfo_NoGitLock() { + ctx := context.Background() + mockPR := mockrepo.NewMockPackageRevision(t.T()) + mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, nil).Once() + + commit, commitTime := externalCommitInfo(ctx, mockPR) + t.Equal("", commit) + t.True(commitTime.IsZero()) +} + +func (t *DbTestSuite) TestExternalCommitInfo_WithGitLock() { + ctx := context.Background() + mockPR := mockrepo.NewMockPackageRevision(t.T()) + mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{ + Git: &kptfilev1.GitLock{Commit: "abc123"}, + }, nil).Once() + + commit, commitTime := externalCommitInfo(ctx, mockPR) + t.Equal("abc123", commit) + t.True(commitTime.IsZero()) +} + +func (t *DbTestSuite) TestExternalCommitInfo_GetLockError() { + ctx := context.Background() + mockPR := mockrepo.NewMockPackageRevision(t.T()) + mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, errors.New("lock error")).Once() + + commit, commitTime := externalCommitInfo(ctx, mockPR) + t.Equal("", commit) + t.True(commitTime.IsZero()) +} + +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) TestExtCommitChangedSincePush() { + now := time.Now() + commit := "abc123" + otherCommit := "def456" + + pr := &dbPackageRevision{} + t.False(extCommitChangedSincePush(pr, "", now)) + + t.False(extCommitChangedSincePush(pr, commit, now)) + + pr.lastPushedCommit = &commit + t.False(extCommitChangedSincePush(pr, commit, now)) + + t.True(extCommitChangedSincePush(pr, otherCommit, now)) + + earlier := now.Add(-time.Second) + pr.lastPushedCommitTimestamp = &now + t.False(extCommitChangedSincePush(pr, otherCommit, earlier)) + + later := now.Add(time.Second) + t.True(extCommitChangedSincePush(pr, otherCommit, later)) +} + +func (t *DbTestSuite) TestCommitTaskForPush() { + commit := "abc123" + pr := &dbPackageRevision{lastPushedCommit: &commit} + task := commitTaskForPush(pr) + t.Require().NotNil(task) + t.Equal(porchapi.TaskTypePush, task.Type) + + pr2 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeRender}}} + t.Nil(commitTaskForPush(pr2)) + + pr3 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeInit}}} + task3 := commitTaskForPush(pr3) + t.Require().NotNil(task3) + t.Equal(porchapi.TaskTypeInit, task3.Type) + + pr4 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeClone}}} + task4 := commitTaskForPush(pr4) + t.Require().NotNil(task4) + t.Equal(porchapi.TaskTypeClone, task4.Type) + + pr5 := &dbPackageRevision{} + t.Nil(commitTaskForPush(pr5)) +} + +func (t *DbTestSuite) TestPrNeedsPushToGit() { + now := time.Now() + + pr := &dbPackageRevision{} + t.True(prNeedsPushToGit(pr)) + + commit := "abc123" + pr.lastPushedCommit = &commit + 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() { + commit := "abc123" + commitTime := time.Now() + dbUpdated := commitTime.Add(-time.Minute) + + existing := &dbPackageRevision{ + lastPushedCommit: &commit, + lastPushedCommitTimestamp: &commitTime, + lastPushedDbUpdated: &dbUpdated, + } + + pr := &dbPackageRevision{} + preservePushMarkersIfUnset(pr, existing) + t.Require().NotNil(pr.lastPushedCommit) + t.Equal(commit, *pr.lastPushedCommit) + t.Equal(&commitTime, pr.lastPushedCommitTimestamp) + t.Equal(&dbUpdated, pr.lastPushedDbUpdated) + + otherCommit := "other" + prWithMarker := &dbPackageRevision{lastPushedCommit: &otherCommit} + preservePushMarkersIfUnset(prWithMarker, existing) + t.Equal(otherCommit, *prWithMarker.lastPushedCommit) + + prEmpty := &dbPackageRevision{} + preservePushMarkersIfUnset(prEmpty, &dbPackageRevision{}) + t.Nil(prEmpty.lastPushedCommit) +} 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/git/git.go b/pkg/externalrepo/git/git.go index 5374008e3..7446e836b 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -1957,15 +1957,32 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos } } + // Read back the committer timestamp recorded on the commit so callers can persist the actual + // git-generated timestamp rather than a locally generated one. + var commitTime time.Time + if !commitHash.IsZero() { + if err := r.sharedDir.withLock(func(repo *git.Repository) error { + commitObj, err := repo.CommitObject(commitHash) + if err != nil { + return err + } + commitTime = commitObj.Committer.When + return nil + }); err != nil { + klog.Warningf("ClosePackageRevisionDraft: could not read commit %s to determine its timestamp: %v", commitHash, err) + } + } + return &gitPackageRevision{ - prKey: d.prKey, - repo: d.repo, - updated: updatedTime, - updatedBy: updatedBy, - ref: newRef, - tree: d.tree, - commit: commitHash, - tasks: d.tasks, + prKey: d.prKey, + repo: d.repo, + updated: updatedTime, + updatedBy: updatedBy, + ref: newRef, + tree: d.tree, + commit: commitHash, + commitTime: commitTime, + tasks: d.tasks, }, nil } diff --git a/pkg/externalrepo/git/package.go b/pkg/externalrepo/git/package.go index d27733de3..74f07edfd 100644 --- a/pkg/externalrepo/git/package.go +++ b/pkg/externalrepo/git/package.go @@ -34,16 +34,17 @@ import ( ) type gitPackageRevision struct { - prKey repository.PackageRevisionKey - repo *gitRepository // repo is repo containing the package - updated time.Time - updatedBy string - ref *plumbing.Reference // ref is the Git reference at which the package exists - tree plumbing.Hash // Cached tree of the package itself, some descendent of commit.Tree() - commit plumbing.Hash // Current version of the package (commit sha) - tasks []porchapi.Task - metadata metav1.ObjectMeta - mutex sync.Mutex + prKey repository.PackageRevisionKey + repo *gitRepository // repo is repo containing the package + updated time.Time + updatedBy string + ref *plumbing.Reference // ref is the Git reference at which the package exists + tree plumbing.Hash // Cached tree of the package itself, some descendent of commit.Tree() + commit plumbing.Hash // Current version of the package (commit sha) + commitTime time.Time // Committer timestamp recorded on commit (zero if unknown) + tasks []porchapi.Task + metadata metav1.ObjectMeta + mutex sync.Mutex } var _ repository.PackageRevision = &gitPackageRevision{} @@ -68,6 +69,10 @@ func (p *gitPackageRevision) Key() repository.PackageRevisionKey { return p.prKey } +func (p *gitPackageRevision) CommitTimestamp() time.Time { + return p.commitTime +} + func (p *gitPackageRevision) GetPackageRevision(ctx context.Context) (*porchapi.PackageRevision, error) { ctx, span := tracer.Start(ctx, "gitPackageRevision::GetPackageRevision", trace.WithAttributes()) defer span.End() diff --git a/pkg/externalrepo/git/package_tree.go b/pkg/externalrepo/git/package_tree.go index 4f17f8370..629560335 100644 --- a/pkg/externalrepo/git/package_tree.go +++ b/pkg/externalrepo/git/package_tree.go @@ -61,6 +61,7 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision var updated time.Time var updatedBy string + var commitTime time.Time // For the published packages on a tag or draft and proposed branches we know that the latest commit // if specific to the package in question. Thus, we can just take the last commit on the tag/branch. @@ -68,6 +69,7 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision if ref != nil && (isTagInLocalRepo(ref.Name()) || isDraftBranchNameInLocal(ref.Name()) || isProposedBranchNameInLocal(ref.Name())) { updated = p.parent.commit.Author.When updatedBy = p.parent.commit.Author.Email + commitTime = p.parent.commit.Committer.When } else { // If we are on the package branch, we can not assume that the last commit // pertains to the package in question. So we scan the git history to find @@ -81,6 +83,7 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision if commit != nil { updated = commit.Author.When updatedBy = commit.Author.Email + commitTime = commit.Committer.When } else { klog.Warningf("Cannot find latest package commit for package %s/%s: %s", p.pkgKey, revisionStr, err) } @@ -122,14 +125,15 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision } return &gitPackageRevision{ - prKey: gitPrKey, - repo: repo, - updated: updated, - updatedBy: updatedBy, - ref: ref, - tree: p.treeHash, - commit: p.parent.commit.Hash, - tasks: tasks, + prKey: gitPrKey, + repo: repo, + updated: updated, + updatedBy: updatedBy, + ref: ref, + tree: p.treeHash, + commit: p.parent.commit.Hash, + commitTime: commitTime, + tasks: tasks, }, nil } diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 58dec9c69..111a3cc03 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -306,6 +306,12 @@ type PackageRevision interface { IsLatestRevision() bool } +// CommitTimeGetter is optionally implemented by PackageRevision implementations that are backed by +// a git commit. CommitTimestamp returns the timestamp recorded on the underlying git commit +type CommitTimeGetter interface { + CommitTimestamp() time.Time +} + // Package is an abstract package. type Package interface { KubeObjectNamespace() string 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..87a2f649c --- /dev/null +++ b/test/e2e/api/db_git_sync_test.go @@ -0,0 +1,469 @@ +// 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 ( + "time" + + porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + suiteutils "github.com/kptdev/porch/test/e2e/suiteutils" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + dbGitTestRepoName = "db-git-test-repo" + dbGitSyncWaitTimeout = 60 * time.Second +) + +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.UpdateAndWaitForRender(&prr) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + 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.UpdateAndWaitForRender(&prr) + 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) TestSyncPullsGitChangeIntoDB() { + 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) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + + time.Sleep(5 * time.Second) + + // 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().True(hasFromGit, + "package resources must contain %q after the external git commit was pulled 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) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + 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.UpdateAndWaitForRender(&prr) + t.Logf("updated resources for draft %s (push expected to fail – repo is archived)", pr.Name) + + // Unarchive and trigger sync. reconcileBothPRs detects dbChanged && + // !extChanged (git is still at initialSHA) and enqueues a push. + t.SetGiteaRepoArchived(giteaRepo, false) + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + newSHA := t.WaitUntilGiteaBranchHasNewCommit(giteaRepo, branchName, initialSHA, dbGitSyncWaitTimeout) + 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) + + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + 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.UpdateAndWaitForRender(&prr) + t.Logf("updated resources in DB (push expected to fail – repo is archived)") + + // Change #2: commit a different file directly to the git branch while the + // repo is still archived. This advances the external commit past + // lastPushedCommitTimestamp, satisfying extChanged = true. + t.SetGiteaRepoArchived(giteaRepo, false) + t.GiteaCommitFileToBranch(giteaRepo, branchName, gitFileKey, + `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.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + + t.WaitUntilGiteaBranchHasNewCommit(giteaRepo, branchName, externalOnlySHA, dbGitSyncWaitTimeout) + + 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/suiteutils/gitea_test_utils.go b/test/e2e/suiteutils/gitea_test_utils.go index d78981d21..2681aa16c 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,25 @@ const ( defaultGiteaLBIP = "172.18.255.200" ) +// GetGiteaURL returns the appropriate Gitea URL based on whether Porch server is running in cluster +func (t *TestSuite) GetGiteaURL() string { + if t.IsPorchServerInCluster() { + return t.GiteaUrl + "/" + t.GiteaUser + "/" + } + return "http://localhost:3000/porch/" +} + +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 +151,283 @@ 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, _ := http.NewRequest("POST", t.GetGiteaApiURL()+"/api/v1/user/repos", 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("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..55787cdeb 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -764,6 +764,97 @@ 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"] = porchapi.Revision2Str(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] +} + +// TriggerRepoSync schedules a one-time sync for the given repository by setting +// spec.sync.runOnceAt to now+7s, then waits for the sync to complete. +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) + + if repo.Spec.Sync == nil { + repo.Spec.Sync = &configapi.RepositorySync{} + } + // The handleRunOnceAt goroutine polls every 5s and requires time.Until(runOnceAt) > 0 + // when it observes the change, so we add 8s of lead time to be safe. + repo.Spec.Sync.RunOnceAt = ptr.To(metav1.NewTime(time.Now().Add(8 * time.Second))) + t.UpdateF(&repo) + + t.Logf("TriggerRepoSync: set runOnceAt for repo %s, waiting for sync to complete", repoName) + t.WaitForNextRepoSync(repoName, timeout) +} + +// WaitForNextRepoSync waits until the Ready condition message changes, indicating a sync cycle completed. +func (t *TestSuite) WaitForNextRepoSync(repoName string, timeout time.Duration) { + t.T().Helper() + repoKey := client.ObjectKey{Namespace: t.Namespace, Name: repoName} + + var repo configapi.Repository + t.GetF(repoKey, &repo) + + currentMsg := "" + for _, cond := range repo.Status.Conditions { + if cond.Type == configapi.RepositoryReady { + currentMsg = cond.Message + break + } + } + + t.Logf("WaitForNextRepoSync: waiting for repo %s condition to change from %q (timeout %v)", repoName, currentMsg, 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 + } + for _, cond := range latest.Status.Conditions { + if cond.Type == configapi.RepositoryReady && cond.Message != currentMsg { + t.Logf("WaitForNextRepoSync: repo %s sync completed (new message=%q)", repoName, cond.Message) + 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, From 7b613b6b1bb210b8575e76cf416cb243680345a0 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Mon, 10 Aug 2026 10:52:26 +0200 Subject: [PATCH 02/13] Fix build and unit test issues Signed-off-by: Rendre Greyling --- pkg/cache/dbcache/dbpackagerevision_test.go | 28 +----- pkg/cache/dbcache/dbpackagerevisionsql.go | 10 +- pkg/cache/dbcache/dbpushtogit_test.go | 104 ++++++-------------- pkg/cache/dbcache/dbrepository_test.go | 14 +-- pkg/cache/dbcache/dbreposync_test.go | 20 ++-- pkg/externalrepo/fake/repository.go | 7 +- test/e2e/api/db_git_sync_test.go | 34 ++++++- test/e2e/suiteutils/suite_utils.go | 2 +- 8 files changed, 89 insertions(+), 130 deletions(-) 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 3acb881eb..299aea854 100644 --- a/pkg/cache/dbcache/dbpackagerevisionsql.go +++ b/pkg/cache/dbcache/dbpackagerevisionsql.go @@ -54,7 +54,7 @@ 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_commit, package_revisions.last_pushed_commit_timestamp, package_revisions.last_pushed_db_updated @@ -130,7 +130,7 @@ 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_commit, package_revisions.last_pushed_commit_timestamp, package_revisions.last_pushed_db_updated @@ -183,7 +183,7 @@ 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_commit, package_revisions.last_pushed_commit_timestamp, package_revisions.last_pushed_db_updated @@ -237,7 +237,7 @@ 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_commit, package_revisions.last_pushed_commit_timestamp, package_revisions.last_pushed_db_updated @@ -382,7 +382,7 @@ 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, , last_pushed_commit, last_pushed_commit_timestamp, last_pushed_db_updated) + 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_commit, last_pushed_commit_timestamp, last_pushed_db_updated) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17) ` diff --git a/pkg/cache/dbcache/dbpushtogit_test.go b/pkg/cache/dbcache/dbpushtogit_test.go index 98f6dc7a9..a748fad90 100644 --- a/pkg/cache/dbcache/dbpushtogit_test.go +++ b/pkg/cache/dbcache/dbpushtogit_test.go @@ -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,21 @@ 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().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.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() @@ -194,36 +181,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 +225,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 +251,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 +269,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 +287,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 +297,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 +307,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 +316,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 +326,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 +340,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_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_test.go b/pkg/cache/dbcache/dbreposync_test.go index e37acd9a7..f586daa35 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" @@ -263,14 +263,17 @@ func (t *DbTestSuite) TestDBRepoSyncWithPushDraftsToGit_DraftOnlyInCacheRemoved( _, err = testRepo.ClosePackageRevisionDraft(ctx, dbPRDraft, 0) t.Require().NoError(err) - // 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) + // Allow the async PushDraftPackageRevision goroutine to finish. + time.Sleep(200 * time.Millisecond) + 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) @@ -1379,9 +1382,9 @@ func (t *DbTestSuite) TestCacheExternalPRs_SkipsRevision0Published() { t.Empty(prList, "revision=0+Published PR should not be cached") } -// TestHandleInCachedOnly_AllowSyncDeletionFalse verifies that no PRs are deleted -// when allowSyncDeletion is false, even when they are cached-only. -func (t *DbTestSuite) TestHandleInCachedOnly_AllowSyncDeletionFalse() { +// 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 @@ -1438,14 +1441,13 @@ func (t *DbTestSuite) TestHandleInCachedOnly_AllowSyncDeletionFalse() { cachedPrMap := repository.PrSlice2Map(prList) inCachedOnly := []repository.PackageRevisionKey{dbPR.Key()} - // Call handleInCachedOnly – should complete without error and NOT delete the PR + // Call handleInCachedOnly – should delete the cached-only workspace PR. err = repoSync.handleInCachedOnly(ctx, cachedPrMap, inCachedOnly) t.Require().NoError(err) - // PR should still be present since allowSyncDeletion=false prListAfter, err := testRepo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{}) t.Require().NoError(err) - t.Len(prListAfter, 2, "PR should not be deleted when allowSyncDeletion=false") + t.Len(prListAfter, 1, "cached-only published workspace PR should be deleted; main branch revision remains") } // TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse verifies that a Draft PR 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/test/e2e/api/db_git_sync_test.go b/test/e2e/api/db_git_sync_test.go index 87a2f649c..0b205e79d 100644 --- a/test/e2e/api/db_git_sync_test.go +++ b/test/e2e/api/db_git_sync_test.go @@ -18,6 +18,7 @@ import ( "time" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" + configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1" suiteutils "github.com/kptdev/porch/test/e2e/suiteutils" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -28,6 +29,31 @@ const ( dbGitSyncWaitTimeout = 60 * time.Second ) +func (t *PorchSuite) updatePRR(repoName string, prr *porchapi.PackageRevisionResources, resourceKeys ...string) { + t.UpdateF(prr) + if t.UsingDBCache { + return + } + var repo configapi.Repository + t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: repoName}, &repo) + if repo.Annotations[configapi.AnnotationKeyV1Alpha2Migration] != configapi.AnnotationValueMigrationEnabled { + return + } + prName := prr.Name + t.Require().Eventually(func() bool { + var latest porchapi.PackageRevisionResources + if err := t.Reader.Get(t.GetContext(), client.ObjectKey{Namespace: t.Namespace, Name: prName}, &latest); err != nil { + return false + } + for _, key := range resourceKeys { + if _, ok := latest.Spec.Resources[key]; !ok { + return false + } + } + return t.CheckRenderError(&latest.Status.RenderStatus) == nil + }, dbGitSyncWaitTimeout, time.Second) +} + func (t *PorchSuite) TestSyncDraftSurvivesSyncWhenInGit() { const ( repoName = dbGitTestRepoName + "-s1" @@ -123,7 +149,7 @@ metadata: data: recovered: "true" ` - t.UpdateAndWaitForRender(&prr) + t.updatePRR(repoName, &prr, "recovery.yaml") t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) @@ -225,7 +251,7 @@ metadata: data: updated-by: concurrent-test ` - t.UpdateAndWaitForRender(&prr) + t.updatePRR(repoName, &prr, newFileKey) t.Logf("updated resources for draft %s (advances updated timestamp)", pr.Name) t.SetGiteaRepoArchived(giteaRepo, false) @@ -379,7 +405,7 @@ metadata: data: updated-by: reconcile-test ` - t.UpdateAndWaitForRender(&prr) + 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. reconcileBothPRs detects dbChanged && @@ -436,7 +462,7 @@ metadata: data: origin: db ` - t.UpdateAndWaitForRender(&prr) + 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 while the diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 55787cdeb..188f5460b 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -782,7 +782,7 @@ func (t *TestSuite) GetPackageRevisionWithFilter(repo, pkgName string, filter Pa "spec.packageName": pkgName, } if filter.Revision != 0 { - fieldSet["spec.revision"] = porchapi.Revision2Str(filter.Revision) + fieldSet["spec.revision"] = strconv.Itoa(filter.Revision) } if filter.Workspace != "" { fieldSet["spec.workspaceName"] = filter.Workspace From e8a591df393ec7f7aef4266d00d8e109f5554863 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Mon, 10 Aug 2026 11:48:59 +0200 Subject: [PATCH 03/13] UT fix Signed-off-by: Rendre Greyling --- pkg/externalrepo/fake/repository_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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)) From e1fdfb8f76d454f0a7cdfcfd0b3b7a8b68c4fe54 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Tue, 11 Aug 2026 15:26:44 +0200 Subject: [PATCH 04/13] Fix e2e tests Signed-off-by: Rendre Greyling --- deployments/porch/9-controllers.yaml | 1 + scripts/deploy/create-deployment-blueprint.sh | 9 ++- test/e2e/api/db_git_sync_test.go | 36 ++++++------ test/e2e/suiteutils/gitea_test_utils.go | 10 +--- test/e2e/suiteutils/suite_utils.go | 57 +++++++++++-------- 5 files changed, 63 insertions(+), 50 deletions(-) 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/scripts/deploy/create-deployment-blueprint.sh b/scripts/deploy/create-deployment-blueprint.sh index c98f64894..3887254c1 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 draft push flags for porch-server and porch-controllers --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 index 0b205e79d..d4cd248eb 100644 --- a/test/e2e/api/db_git_sync_test.go +++ b/test/e2e/api/db_git_sync_test.go @@ -15,12 +15,13 @@ package api import ( + "context" "time" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" - configapi "github.com/kptdev/porch/api/porchconfig/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" ) @@ -29,29 +30,29 @@ const ( dbGitSyncWaitTimeout = 60 * time.Second ) -func (t *PorchSuite) updatePRR(repoName string, prr *porchapi.PackageRevisionResources, resourceKeys ...string) { +func (t *PorchSuite) updatePRR(_ string, prr *porchapi.PackageRevisionResources, resourceKeys ...string) { + t.T().Helper() t.UpdateF(prr) - if t.UsingDBCache { - return - } - var repo configapi.Repository - t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: repoName}, &repo) - if repo.Annotations[configapi.AnnotationKeyV1Alpha2Migration] != configapi.AnnotationValueMigrationEnabled { - return - } + prName := prr.Name - t.Require().Eventually(func() bool { + err := wait.PollUntilContextTimeout(t.GetContext(), time.Second, dbGitSyncWaitTimeout, true, func(ctx context.Context) (bool, error) { var latest porchapi.PackageRevisionResources - if err := t.Reader.Get(t.GetContext(), client.ObjectKey{Namespace: t.Namespace, Name: prName}, &latest); err != nil { - return false + 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 + return false, nil } } - return t.CheckRenderError(&latest.Status.RenderStatus) == nil - }, dbGitSyncWaitTimeout, time.Second) + 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) TestSyncDraftSurvivesSyncWhenInGit() { @@ -469,7 +470,8 @@ data: // repo is still archived. This advances the external commit past // lastPushedCommitTimestamp, satisfying extChanged = true. t.SetGiteaRepoArchived(giteaRepo, false) - t.GiteaCommitFileToBranch(giteaRepo, branchName, gitFileKey, + gitFilePath := packageName + "/" + gitFileKey + t.GiteaCommitFileToBranch(giteaRepo, branchName, gitFilePath, `apiVersion: v1 kind: ConfigMap metadata: diff --git a/test/e2e/suiteutils/gitea_test_utils.go b/test/e2e/suiteutils/gitea_test_utils.go index 2681aa16c..ef1b53ed3 100644 --- a/test/e2e/suiteutils/gitea_test_utils.go +++ b/test/e2e/suiteutils/gitea_test_utils.go @@ -43,14 +43,6 @@ const ( defaultGiteaLBIP = "172.18.255.200" ) -// GetGiteaURL returns the appropriate Gitea URL based on whether Porch server is running in cluster -func (t *TestSuite) GetGiteaURL() string { - if t.IsPorchServerInCluster() { - return t.GiteaUrl + "/" + t.GiteaUser + "/" - } - return "http://localhost:3000/porch/" -} - func (t *TestSuite) GetGiteaApiURL() string { if t.GiteaUrl == GiteaClusterURL { return "http://localhost:3000" @@ -180,7 +172,7 @@ func (t *TestSuite) CreateGiteaRepoNoCleanup(repoName string) string { } t.Logf("CreateGiteaRepoNoCleanup: created repo %q", repoName) - return t.GetGiteaURL() + repoName + ".git" + return t.getGiteaURL() + repoName + ".git" } // DeleteGiteaRepo deletes a Gitea repository owned by t.GiteaUser. diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 188f5460b..50c15dd52 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -798,8 +798,6 @@ func (t *TestSuite) GetPackageRevisionWithFilter(repo, pkgName string, filter Pa return &prList.Items[0] } -// TriggerRepoSync schedules a one-time sync for the given repository by setting -// spec.sync.runOnceAt to now+7s, then waits for the sync to complete. func (t *TestSuite) TriggerRepoSync(repoName string, timeout time.Duration) { t.T().Helper() repoKey := client.ObjectKey{Namespace: t.Namespace, Name: repoName} @@ -807,46 +805,59 @@ func (t *TestSuite) TriggerRepoSync(repoName string, timeout time.Duration) { var repo configapi.Repository t.GetF(repoKey, &repo) + baselineLastSync := time.Time{} + if repo.Status.LastFullSyncTime != nil { + baselineLastSync = repo.Status.LastFullSyncTime.Time + } + if repo.Spec.Sync == nil { repo.Spec.Sync = &configapi.RepositorySync{} } - // The handleRunOnceAt goroutine polls every 5s and requires time.Until(runOnceAt) > 0 - // when it observes the change, so we add 8s of lead time to be safe. - repo.Spec.Sync.RunOnceAt = ptr.To(metav1.NewTime(time.Now().Add(8 * time.Second))) + // 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 = ptr.To(runOnceAt) t.UpdateF(&repo) t.Logf("TriggerRepoSync: set runOnceAt for repo %s, waiting for sync to complete", repoName) - t.WaitForNextRepoSync(repoName, timeout) + t.WaitForNextRepoSync(repoName, timeout, baselineLastSync, runOnceAt.Time) } -// WaitForNextRepoSync waits until the Ready condition message changes, indicating a sync cycle completed. -func (t *TestSuite) WaitForNextRepoSync(repoName string, timeout time.Duration) { +func (t *TestSuite) WaitForNextRepoSync(repoName string, timeout time.Duration, baselineLastSync, triggeredRunOnceAt time.Time) { t.T().Helper() repoKey := client.ObjectKey{Namespace: t.Namespace, Name: repoName} - var repo configapi.Repository - t.GetF(repoKey, &repo) - - currentMsg := "" - for _, cond := range repo.Status.Conditions { - if cond.Type == configapi.RepositoryReady { - currentMsg = cond.Message - break - } - } - - t.Logf("WaitForNextRepoSync: waiting for repo %s condition to change from %q (timeout %v)", repoName, currentMsg, timeout) + 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.Message != currentMsg { - t.Logf("WaitForNextRepoSync: repo %s sync completed (new message=%q)", repoName, cond.Message) - return true, nil + 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 }) From ce94aa6bdc9fa958aad7027a2a3efc6cfad2a78c Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Tue, 11 Aug 2026 21:44:39 +0200 Subject: [PATCH 05/13] Enable push drafts in ci and fix gosec Signed-off-by: Rendre Greyling --- make/deploy.mk | 4 ++++ pkg/cache/dbcache/dbpackagerevision.go | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) 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 5d09c66be..d10f5419e 100644 --- a/pkg/cache/dbcache/dbpackagerevision.go +++ b/pkg/cache/dbcache/dbpackagerevision.go @@ -557,7 +557,8 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap pr.extPRID = pushedPRExtID if pushedPRExtID.Git != nil && pushedPRExtID.Git.Commit != "" { - pr.lastPushedCommit = new(pushedPRExtID.Git.Commit) + commit := pushedPRExtID.Git.Commit + pr.lastPushedCommit = &commit if commitTimestamp.IsZero() { commitTimestamp = time.Now() } From 05223134bc0a1a9257952f2b19e8dbb2f328a177 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Wed, 12 Aug 2026 09:35:17 +0200 Subject: [PATCH 06/13] Simplify db sync by only syncing db to upstream Signed-off-by: Rendre Greyling --- api/sql/porch-db-1.6.0-1.6.4.sql | 4 +- api/sql/porch-db-1.6.4-1.6.0.sql | 4 +- api/sql/porch-db.sql | 2 - .../porch/3-porch-postgres-bundle.yaml | 2 - pkg/cache/dbcache/dbpackagerevision.go | 63 ++++------- pkg/cache/dbcache/dbpackagerevisionsql.go | 67 +++-------- pkg/cache/dbcache/dbpushtogit.go | 51 +++------ pkg/cache/dbcache/dbrepository.go | 2 +- pkg/cache/dbcache/dbreposync.go | 92 ++------------- pkg/cache/dbcache/dbreposync_test.go | 104 +---------------- pkg/cache/dbcache/util.go | 41 +++---- pkg/cache/dbcache/util_test.go | 107 +++++------------- pkg/externalrepo/git/git.go | 33 ++---- pkg/externalrepo/git/package.go | 5 - pkg/externalrepo/git/package_tree.go | 20 ++-- pkg/repository/repository.go | 6 - test/e2e/api/db_git_sync_test.go | 49 ++++---- 17 files changed, 155 insertions(+), 497 deletions(-) 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 index 4eddd3000..a11ccb9dd 100644 --- a/api/sql/porch-db-1.6.0-1.6.4.sql +++ b/api/sql/porch-db-1.6.0-1.6.4.sql @@ -15,6 +15,4 @@ limitations under the License. */ ALTER TABLE package_revisions - ADD COLUMN IF NOT EXISTS last_pushed_commit TEXT, - ADD COLUMN IF NOT EXISTS last_pushed_commit_timestamp TIMESTAMP, - ADD COLUMN IF NOT EXISTS last_pushed_db_updated TIMESTAMP; \ No newline at end of file + 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 index b8bac1796..7412d3645 100644 --- a/api/sql/porch-db-1.6.4-1.6.0.sql +++ b/api/sql/porch-db-1.6.4-1.6.0.sql @@ -15,6 +15,4 @@ limitations under the License. */ ALTER TABLE package_revisions - DROP COLUMN IF EXISTS last_pushed_commit, - DROP COLUMN IF EXISTS last_pushed_commit_timestamp, - DROP COLUMN IF EXISTS last_pushed_db_updated; \ No newline at end of file + DROP COLUMN IF EXISTS last_pushed_db_updated; diff --git a/api/sql/porch-db.sql b/api/sql/porch-db.sql index 832285676..cd3c9391c 100644 --- a/api/sql/porch-db.sql +++ b/api/sql/porch-db.sql @@ -88,8 +88,6 @@ 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_commit TEXT, - last_pushed_commit_timestamp TIMESTAMP, last_pushed_db_updated TIMESTAMP, PRIMARY KEY (k8s_name_space, k8s_name), CONSTRAINT fk_package diff --git a/deployments/porch/3-porch-postgres-bundle.yaml b/deployments/porch/3-porch-postgres-bundle.yaml index 47a703977..7a41ae95c 100644 --- a/deployments/porch/3-porch-postgres-bundle.yaml +++ b/deployments/porch/3-porch-postgres-bundle.yaml @@ -356,8 +356,6 @@ data: kptfile_status TEXT NOT NULL DEFAULT '{}', resources_size BIGINT NOT NULL DEFAULT 0, upstream_ref_name TEXT NOT NULL DEFAULT '', - last_pushed_commit TEXT, - last_pushed_commit_timestamp TIMESTAMP, last_pushed_db_updated TIMESTAMP, PRIMARY KEY (k8s_name_space, k8s_name), CONSTRAINT fk_package diff --git a/pkg/cache/dbcache/dbpackagerevision.go b/pkg/cache/dbcache/dbpackagerevision.go index d10f5419e..2c7815c24 100644 --- a/pkg/cache/dbcache/dbpackagerevision.go +++ b/pkg/cache/dbcache/dbpackagerevision.go @@ -75,33 +75,21 @@ 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 - - // lastPushedCommit is the git commit hash of the last successful push of this revision to git. - // A nil value means the revision has never been (successfully) pushed to git. - lastPushedCommit *string - - // lastPushedCommitTimestamp is the timestamp associated with the last commit pushed to git. - // It is used by conflict resolution to reason about the git side of the last push. - lastPushedCommitTimestamp *time.Time - - // lastPushedDbUpdated is the value of the DB `updated` column at the time of the last successful - // push to git. It is used to detect whether the revision content has changed since it was last - // pushed, and by conflict resolution to reason about the DB side of the last push. + 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 } @@ -491,18 +479,17 @@ func (pr *dbPackageRevision) copyToThis(otherPr *dbPackageRevision) { pr.tasks = otherPr.tasks pr.resources = otherPr.resources pr.resourcesSizeBytes = otherPr.resourcesSizeBytes - pr.lastPushedCommit = otherPr.lastPushedCommit - pr.lastPushedCommitTimestamp = otherPr.lastPushedCommitTimestamp pr.lastPushedDbUpdated = otherPr.lastPushedDbUpdated } func preservePushMarkersIfUnset(pr, existing *dbPackageRevision) { - if pr.lastPushedCommit != nil || existing.lastPushedCommit == nil { + if pr.lastPushedDbUpdated != nil || existing.lastPushedDbUpdated == nil { return } - pr.lastPushedCommit = existing.lastPushedCommit - pr.lastPushedCommitTimestamp = existing.lastPushedCommitTimestamp 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 { @@ -547,7 +534,7 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap pr.pkgRevKey.Revision = latestRev + 1 pr.lifecycle = newLifecycle - pushedPRExtID, commitTimestamp, err := PushPublishedPackageRevision(ctx, pr.repo.externalRepo, pr, pr.repo.pushDraftsToGit, pr.lastPushedCommit != nil) + 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 @@ -556,14 +543,8 @@ func (pr *dbPackageRevision) publishPR(ctx context.Context, newLifecycle porchap } pr.extPRID = pushedPRExtID - if pushedPRExtID.Git != nil && pushedPRExtID.Git.Commit != "" { - commit := pushedPRExtID.Git.Commit - pr.lastPushedCommit = &commit - if commitTimestamp.IsZero() { - commitTimestamp = time.Now() - } - pr.lastPushedCommitTimestamp = &commitTimestamp - } + 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/dbpackagerevisionsql.go b/pkg/cache/dbcache/dbpackagerevisionsql.go index 299aea854..0f18983e1 100644 --- a/pkg/cache/dbcache/dbpackagerevisionsql.go +++ b/pkg/cache/dbcache/dbpackagerevisionsql.go @@ -55,8 +55,6 @@ func pkgRevReadFromDB(ctx context.Context, prk repository.PackageRevisionKey, re package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size, - package_revisions.last_pushed_commit, - package_revisions.last_pushed_commit_timestamp, 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 @@ -131,8 +129,6 @@ func pkgRevListPRsFromDB(ctx context.Context, filter repository.ListPackageRevis package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size, - package_revisions.last_pushed_commit, - package_revisions.last_pushed_commit_timestamp, package_revisions.last_pushed_db_updated FROM package_revisions INNER JOIN packages @@ -184,8 +180,6 @@ func pkgRevReadPRsFromDB(ctx context.Context, pk repository.PackageKey) ([]*dbPa package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size, - package_revisions.last_pushed_commit, - package_revisions.last_pushed_commit_timestamp, 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 @@ -238,8 +232,6 @@ func pkgRevReadLatestPRFromDB(ctx context.Context, pk repository.PackageKey) (*d package_revisions.tasks, package_revisions.kptfile_status, package_revisions.resources_size, - package_revisions.last_pushed_commit, - package_revisions.last_pushed_commit_timestamp, 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 @@ -308,8 +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 lastPushedCommit sql.NullString - var lastPushedCommitTimestamp, lastPushedDbUpdated sql.NullTime + var lastPushedDbUpdated sql.NullTime err := rows.Scan( &pkgRev.pkgRevKey.PkgKey.RepoKey.Namespace, @@ -331,8 +322,6 @@ func pkgRevScanRowsFromDB(ctx context.Context, rows *sql.Rows) ([]*dbPackageRevi &tasks, &kptfileStatusJSON, &pkgRev.resourcesSizeBytes, - &lastPushedCommit, - &lastPushedCommitTimestamp, &lastPushedDbUpdated) if err != nil { @@ -340,14 +329,6 @@ func pkgRevScanRowsFromDB(ctx context.Context, rows *sql.Rows) ([]*dbPackageRevi return nil, err } - if lastPushedCommit.Valid { - commit := lastPushedCommit.String - pkgRev.lastPushedCommit = &commit - } - if lastPushedCommitTimestamp.Valid { - commitTimestamp := lastPushedCommitTimestamp.Time - pkgRev.lastPushedCommitTimestamp = &commitTimestamp - } if lastPushedDbUpdated.Valid { dbUpdated := lastPushedDbUpdated.Time pkgRev.lastPushedDbUpdated = &dbUpdated @@ -382,21 +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, last_pushed_commit, last_pushed_commit_timestamp, last_pushed_db_updated) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17) + 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) - lastPushedCommit := lastPushedCommitAsNullString(pr) - lastPushedCommitTimestamp := lastPushedCommitTimestampAsNullTime(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), lastPushedCommit, lastPushedCommitTimestamp, lastPushedDbUpdated); 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) @@ -419,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, last_pushed_commit=$15, last_pushed_commit_timestamp=$16, last_pushed_db_updated=$17 + 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, last_pushed_commit, last_pushed_commit_timestamp, last_pushed_db_updated + 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, $16, $17 + $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 @@ -443,23 +422,19 @@ func pkgRevUpdateDB(ctx context.Context, pr *dbPackageRevision, updateResources kptfile_status = EXCLUDED.kptfile_status, resources_size = EXCLUDED.resources_size, upstream_ref_name = EXCLUDED.upstream_ref_name, - last_pushed_commit = EXCLUDED.last_pushed_commit, - last_pushed_commit_timestamp = EXCLUDED.last_pushed_commit_timestamp, last_pushed_db_updated = EXCLUDED.last_pushed_db_updated; ` } klog.V(6).Infof("pkgRevUpdateDB: running query %q on package revision %+v", sqlStatement, pr) - lastPushedCommit := lastPushedCommitAsNullString(pr) - lastPushedCommitTimestamp := lastPushedCommitTimestampAsNullTime(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), lastPushedCommit, lastPushedCommitTimestamp, lastPushedDbUpdated) + 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 { @@ -748,20 +723,6 @@ func backfillUpstreamRefName(ctx context.Context) error { return nil } -func lastPushedCommitAsNullString(pr *dbPackageRevision) sql.NullString { - if pr.lastPushedCommit == nil { - return sql.NullString{} - } - return sql.NullString{Valid: true, String: *pr.lastPushedCommit} -} - -func lastPushedCommitTimestampAsNullTime(pr *dbPackageRevision) sql.NullTime { - if pr.lastPushedCommitTimestamp == nil { - return sql.NullTime{} - } - return sql.NullTime{Valid: true, Time: *pr.lastPushedCommitTimestamp} -} - func lastPushedDbUpdatedAsNullTime(pr *dbPackageRevision) sql.NullTime { if pr.lastPushedDbUpdated == nil { return sql.NullTime{} @@ -769,18 +730,18 @@ func lastPushedDbUpdatedAsNullTime(pr *dbPackageRevision) sql.NullTime { return sql.NullTime{Valid: true, Time: *pr.lastPushedDbUpdated} } -// pkgRevSetLastPushedInDB records the last successfully pushed git commit (and its timestamp) for a -// package revision without touching the `updated`/`updatedby` columns. -func pkgRevSetLastPushedInDB(ctx context.Context, prk repository.PackageRevisionKey, commit string, commitTimestamp time.Time, expectedUpdated time.Time) (bool, error) { +// 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 last_pushed_commit=$3, last_pushed_commit_timestamp=$4, last_pushed_db_updated=$5 - WHERE k8s_name_space=$1 AND k8s_name=$2 AND updated=$5 + 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(), commit, commitTimestamp, expectedUpdated) + 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 diff --git a/pkg/cache/dbcache/dbpushtogit.go b/pkg/cache/dbcache/dbpushtogit.go index 82ef6c6a8..6a4c172be 100644 --- a/pkg/cache/dbcache/dbpushtogit.go +++ b/pkg/cache/dbcache/dbpushtogit.go @@ -18,7 +18,6 @@ import ( "context" "database/sql" "fmt" - "time" kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" @@ -29,7 +28,7 @@ import ( "k8s.io/klog/v2" ) -func PushPublishedPackageRevision(ctx context.Context, repo repository.Repository, pr repository.PackageRevision, pushDraftsToGit, existingGitBranch bool) (kptfilev1.Locator, time.Time, error) { +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() @@ -48,17 +47,17 @@ func PushPublishedPackageRevision(ctx context.Context, repo repository.Repositor prLifecycle := pr.Lifecycle(ctx) if prLifecycle != porchapi.PackageRevisionLifecyclePublished { - return kptfilev1.Locator{}, time.Time{}, fmt.Errorf("cannot push package revision %+v, package revision lifecycle is %q, it should be \"Published\"", pr.Key(), prLifecycle) + 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{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get API definition:", pr.Key(), repo.Key()) + 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{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not get package revision resources:", pr.Key(), repo.Key()) + 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()) } commitTask := &porchapi.Task{Type: porchapi.TaskTypePush} @@ -80,11 +79,11 @@ func PushPublishedPackageRevision(ctx context.Context, repo repository.Repositor if err == nil && len(existingPRs) > 0 { draft, err = repo.UpdatePackageRevision(ctx, existingPRs[0]) if err != nil { - return kptfilev1.Locator{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update existing package revision:", pr.Key(), repo.Key()) + 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, commitTask); err != nil { - return kptfilev1.Locator{}, time.Time{}, 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()) + 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 } @@ -93,36 +92,29 @@ func PushPublishedPackageRevision(ctx context.Context, repo repository.Repositor if !foundExisting { draft, err = repo.CreatePackageRevisionDraft(ctx, apiPr) if err != nil { - return kptfilev1.Locator{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not create package revision draft:", pr.Key(), repo.Key()) + 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, commitTask); err != nil { - return kptfilev1.Locator{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not update package revision resources:", pr.Key(), repo.Key()) + 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{}, time.Time{}, 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()) + 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{}, time.Time{}, pkgerrors.Wrapf(err, "push of package revision %+v to repository %+v failed, could not close package revision draft:", pr.Key(), repo.Key()) + 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{}, time.Time{}, pkgerrors.Wrapf(err, "read of upstream lock for package revision %+v pushed to repository %+v failed", pr.Key(), repo.Key()) + return kptfilev1.Locator{}, pkgerrors.Wrapf(err, "read of upstream lock for package revision %+v pushed to repository %+v failed", pr.Key(), repo.Key()) } - // Capture the actual git commit timestamp when the backend exposes it so callers can persist - // the git-generated timestamp rather than a locally generated one. - var commitTimestamp time.Time - if ctg, ok := pushedPR.(repository.CommitTimeGetter); ok { - commitTimestamp = ctg.CommitTimestamp() - } - - return pushedPRUpstreamLock, commitTimestamp, nil + return pushedPRUpstreamLock, nil } func PushDraftPackageRevision(ctx context.Context, repoKey repository.RepositoryKey, pr *dbPackageRevision) { @@ -203,30 +195,19 @@ func PushDraftPackageRevision(ctx context.Context, repoKey repository.Repository return } - commit := "" - if pushedLock.Git != nil { - commit = pushedLock.Git.Commit - } - - commitTimestamp := time.Now() - if ctg, ok := pushedGitPR.(repository.CommitTimeGetter); ok { - if gitTime := ctg.CommitTimestamp(); !gitTime.IsZero() { - commitTimestamp = gitTime - } - } - - recorded, err := pkgRevSetLastPushedInDB(ctx, prKey, commit, commitTimestamp, updatedBeforePush) + recorded, err := pkgRevSetLastPushedInDB(ctx, prKey, pushedLock, updatedBeforePush) if err != nil { - klog.Warningf("PushDraftPackageRevision: repo %+v: failed to record last_pushed_commit for %+v: %v", repoKey, prKey, err) + 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 last_pushed_commit — next sync will retry with fresh data", + 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) } diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 0cb631b3c..3647f6d3f 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -228,7 +228,7 @@ 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, }, } diff --git a/pkg/cache/dbcache/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index 132844723..7bf192967 100644 --- a/pkg/cache/dbcache/dbreposync.go +++ b/pkg/cache/dbcache/dbreposync.go @@ -29,7 +29,6 @@ import ( "github.com/kptdev/porch/pkg/repository" pkgerrors "github.com/pkg/errors" "go.opentelemetry.io/otel/trace" - "k8s.io/apimachinery/pkg/watch" "k8s.io/klog/v2" ) @@ -109,7 +108,7 @@ func (s *repositorySync) sync(ctx context.Context) (repositorySyncStats, error) } if s.repo.pushDraftsToGit { - s.reconcileBothPRs(ctx, cachedPrMap, externalPrMap, inBoth) + s.handleInBoth(ctx, cachedPrMap, inBoth) } return repositorySyncStats{ @@ -277,12 +276,12 @@ func (s *repositorySync) handleInCachedOnly(ctx context.Context, cachedPrMap map if dbPkgRev, ok := dbPR.(*dbPackageRevision); ok && (dbPkgRev.lifecycle == porchapi.PackageRevisionLifecycleDraft || dbPkgRev.lifecycle == porchapi.PackageRevisionLifecycleProposed) && - dbPkgRev.lastPushedCommit == nil { + !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 (last_pushed_commit is null)", s.repo.Key(), dbPkgRev.lifecycle, dbPRKey) + 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 } @@ -323,7 +322,7 @@ func (s *repositorySync) deleteCachedOnlyPR(ctx context.Context, dbPRKey reposit } if (freshPR.lifecycle == porchapi.PackageRevisionLifecycleDraft || freshPR.lifecycle == porchapi.PackageRevisionLifecycleProposed) && - freshPR.lastPushedCommit == nil { + !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 } @@ -348,8 +347,8 @@ func (s *repositorySync) deleteCachedOnlyPR(ctx context.Context, dbPRKey reposit return nil } -func (s *repositorySync) reconcileBothPRs(ctx context.Context, cachedPrMap, externalPrMap map[repository.PackageRevisionKey]repository.PackageRevision, inBoth []repository.PackageRevisionKey) { - ctx, span := tracer.Start(ctx, "Repository::reconcileBothPRs", trace.WithAttributes()) +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 { @@ -362,90 +361,13 @@ func (s *repositorySync) reconcileBothPRs(ctx context.Context, cachedPrMap, exte continue } - extPR := externalPrMap[prKey] - externalCommit, externalCommitTime := externalCommitInfo(ctx, extPR) - - dbChanged := dbContentChangedSincePush(cachedPR) - extChanged := extCommitChangedSincePush(cachedPR, externalCommit, externalCommitTime) - - switch { - case !dbChanged && !extChanged: - // In sync - case dbChanged && !extChanged: + 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) - case !dbChanged && extChanged: - klog.Infof("repositorySync %+v: reconcile %+v: incoming git commit %q, pulling into DB", s.repo.Key(), prKey, externalCommit) - if err := s.pullExternalIntoDB(ctx, cachedPR, extPR, externalCommit, externalCommitTime); err != nil { - klog.Warningf("repositorySync %+v: reconcile %+v: failed to pull incoming commit into DB: %v", s.repo.Key(), prKey, err) - } - default: - // Both DB and External Git has changed which may result in a conflict - // TODO decide if conflict resolution is at all necessary - for now DB will always overwrite Git changes if both were changed - s.enqueuePush(ctx, cachedPR) } } } -func (s *repositorySync) pullExternalIntoDB(ctx context.Context, cachedPR *dbPackageRevision, extPR repository.PackageRevision, externalCommit string, externalCommitTime time.Time) error { - prKey := cachedPR.Key() - - extAPIPR, err := extPR.GetPackageRevision(ctx) - if err != nil { - return pkgerrors.Wrapf(err, "failed to get external package revision %+v", prKey) - } - - extPRResources, err := extPR.GetResources(ctx) - if err != nil { - return pkgerrors.Wrapf(err, "failed to get resources for external package revision %+v", prKey) - } - resources, resourcesSize := s.sanitizeResources(prKey, extPRResources) - - _, extPRUpstreamLock, _ := extPR.GetLock(ctx) - - pkgMutex := getOrInsertPkgLock(prKey.PKey()) - pkgMutex.Lock() - defer func() { - pkgMutex.Unlock() - deletePkgLock(prKey.PKey()) - }() - - cachedPR.meta = extAPIPR.ObjectMeta - cachedPR.spec = &extAPIPR.Spec - cachedPR.lifecycle = extAPIPR.Spec.Lifecycle - - if len(extAPIPR.Spec.Tasks) > 0 || len(cachedPR.tasks) == 0 { - cachedPR.tasks = extAPIPR.Spec.Tasks - } else { - klog.Warningf("repositorySync %+v: pullExternalIntoDB: external package revision %+v has no tasks, keeping %d cached task(s)", - s.repo.Key(), prKey, len(cachedPR.tasks)) - } - - cachedPR.resources = resources - cachedPR.extPRID = extPRUpstreamLock - cachedPR.resourcesSizeBytes = resourcesSize - - commit := externalCommit - commitTime := externalCommitTime - cachedPR.lastPushedCommit = &commit - cachedPR.lastPushedCommitTimestamp = &commitTime - - if err := pkgRevUpdateDB(ctx, cachedPR, true); err != nil { - return pkgerrors.Wrapf(err, "failed to update cached package revision %+v from external repo", prKey) - } - - dbUpdated := cachedPR.updated - cachedPR.lastPushedDbUpdated = &dbUpdated - if _, err := pkgRevSetLastPushedInDB(ctx, prKey, commit, commitTime, cachedPR.updated); err != nil { - klog.Warningf("repositorySync %+v: pullExternalIntoDB: failed to record last_pushed markers for %+v: %v", s.repo.Key(), prKey, err) - } - - sent := cachedPR.repo.repoPRChangeNotifier.NotifyPackageRevisionChange(watch.Modified, cachedPR) - klog.Infof("DB cache %+v: sent %d notifications for package revision %+v updated from external repo", s.repo.Key(), sent, prKey) - - return nil -} - 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 f586daa35..ba7a04d3e 100644 --- a/pkg/cache/dbcache/dbreposync_test.go +++ b/pkg/cache/dbcache/dbreposync_test.go @@ -1451,7 +1451,7 @@ func (t *DbTestSuite) TestHandleInCachedOnly_DeletesPublishedCachedOnly() { } // TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse verifies that a Draft PR -// with nil lastPushedCommit is not deleted when pushDraftsToGit=false. +// 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 @@ -1476,7 +1476,7 @@ func (t *DbTestSuite) TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse // pushDraftsToGit is false (default zero value) } - // Create a Draft PR (lastPushedCommit will be nil) + // Create a Draft PR (lastPushedDbUpdated will be nil) newPRDef := porchapi.PackageRevision{ Spec: porchapi.PackageRevisionSpec{ RepositoryName: "draftkeep-repo", @@ -1498,9 +1498,9 @@ func (t *DbTestSuite) TestHandleInCachedOnly_DraftNotPushed_PushDraftsToGitFalse t.Require().NoError(err) t.Len(prList, 1, "Draft PR should exist before calling handleInCachedOnly") - // Build cachedPrMap directly with the dbPackageRevision (which has nil lastPushedCommit) + // Build cachedPrMap directly with the dbPackageRevision (which has not been pushed yet) cachedPRTyped := dbPR.(*dbPackageRevision) - t.Nil(cachedPRTyped.lastPushedCommit, "lastPushedCommit should be nil for a new draft") + t.False(hasBeenPushedToGit(cachedPRTyped), "new draft should not have been pushed to git") cachedPrMap := map[repository.PackageRevisionKey]repository.PackageRevision{ dbPR.Key(): cachedPRTyped, @@ -1629,99 +1629,3 @@ func (t *DbTestSuite) TestDeleteCachedOnlyPR_ChangedSinceSnapshot() { t.Require().NoError(err) t.Len(prListAfter, 2, "PR should not be deleted when snapshot is stale") } - -// TestPullExternalIntoDB verifies that pullExternalIntoDB correctly updates a cached -// Draft PR with data from the external repository. -func (t *DbTestSuite) TestPullExternalIntoDB() { - ctx := t.Context() - externalrepo.ExternalRepoInUnitTestMode = true - - testRepo := t.createTestRepo("pull-ns", "pull-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 a Draft PR in the DB - newPRDef := porchapi.PackageRevision{ - Spec: porchapi.PackageRevisionSpec{ - RepositoryName: "pull-repo", - PackageName: "pull-pkg", - WorkspaceName: "pull-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) - - cachedPR := dbPR.(*dbPackageRevision) - t.Nil(cachedPR.lastPushedCommit, "new draft should have no lastPushedCommit") - - // Construct the external PR with updated content - updatedPRDef := &porchapi.PackageRevision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pull-pr-updated", - Namespace: "pull-ns", - CreationTimestamp: metav1.Now(), - }, - Spec: porchapi.PackageRevisionSpec{ - RepositoryName: "pull-repo", - PackageName: "pull-pkg", - WorkspaceName: "pull-ws", - Lifecycle: porchapi.PackageRevisionLifecycleDraft, - }, - } - - updatedResources := &porchapi.PackageRevisionResources{ - Spec: porchapi.PackageRevisionResourcesSpec{ - Resources: map[string]string{ - "Kptfile": "apiVersion: kpt.dev/v1\nkind: Kptfile\n", - "updated-config.yaml": "newKey: newValue\n", - }, - }, - } - - fakeExtPR := &fake.FakePackageRevision{ - PrKey: cachedPR.Key(), - PackageRevision: updatedPRDef, - PackageLifecycle: porchapi.PackageRevisionLifecycleDraft, - Resources: updatedResources, - Kptfile: kptfilev1.KptFile{ - Upstream: &kptfilev1.Upstream{}, - UpstreamLock: &kptfilev1.Locator{}, - }, - } - - externalCommit := "abc123deadbeef" - externalCommitTime := time.Now() - - // pullExternalIntoDB should update the cached PR with the external data - err = repoSync.pullExternalIntoDB(ctx, cachedPR, fakeExtPR, externalCommit, externalCommitTime) - t.Require().NoError(err) - - // Verify the cached PR was updated with the external commit info - t.Require().NotNil(cachedPR.lastPushedCommit, "lastPushedCommit should be set after pull") - t.Equal(externalCommit, *cachedPR.lastPushedCommit) - - // Read back from DB to confirm persistence - freshPR, err := pkgRevReadFromDB(ctx, cachedPR.Key(), false) - t.Require().NoError(err) - t.Require().NotNil(freshPR.lastPushedCommit, "lastPushedCommit should be persisted in DB") - t.Equal(externalCommit, *freshPR.lastPushedCommit) -} diff --git a/pkg/cache/dbcache/util.go b/pkg/cache/dbcache/util.go index 265b00339..6bcecca8e 100644 --- a/pkg/cache/dbcache/util.go +++ b/pkg/cache/dbcache/util.go @@ -15,17 +15,18 @@ package dbcache import ( - "context" "encoding/json" "os/user" "sync" - "time" + 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 { @@ -95,18 +96,19 @@ func deletePkgLock(pkgKey repository.PackageKey) { globalLockManager.deleteLock(pkgKey.String()) } -func externalCommitInfo(ctx context.Context, extPR repository.PackageRevision) (string, time.Time) { - var commit string - if _, lock, err := extPR.GetLock(ctx); err == nil && lock.Git != nil { - commit = lock.Git.Commit - } +func extPRCommit(pr *dbPackageRevision) string { + return extPRCommitFromLocator(pr.extPRID) +} - var commitTime time.Time - if ctg, ok := extPR.(repository.CommitTimeGetter); ok { - commitTime = ctg.CommitTimestamp() +func extPRCommitFromLocator(loc kptfile.Locator) string { + if loc.Git != nil { + return loc.Git.Commit } + return "" +} - return commit, commitTime +func hasBeenPushedToGit(pr *dbPackageRevision) bool { + return pr.lastPushedDbUpdated != nil } func dbContentChangedSincePush(pr *dbPackageRevision) bool { @@ -116,21 +118,8 @@ func dbContentChangedSincePush(pr *dbPackageRevision) bool { return !pr.updated.Equal(*pr.lastPushedDbUpdated) } -func extCommitChangedSincePush(pr *dbPackageRevision, externalCommit string, externalCommitTime time.Time) bool { - if externalCommit == "" || pr.lastPushedCommit == nil { - return false - } - if externalCommit == *pr.lastPushedCommit { - return false - } - if pr.lastPushedCommitTimestamp == nil { - return true - } - return externalCommitTime.After(*pr.lastPushedCommitTimestamp) -} - func commitTaskForPush(pr *dbPackageRevision) *porchapi.Task { - if pr.lastPushedCommit != nil { + if hasBeenPushedToGit(pr) { return &porchapi.Task{Type: porchapi.TaskTypePush} } @@ -144,7 +133,7 @@ func commitTaskForPush(pr *dbPackageRevision) *porchapi.Task { } func prNeedsPushToGit(pr *dbPackageRevision) bool { - if pr.lastPushedCommit == nil || pr.lastPushedDbUpdated == nil { + 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 01c7a7619..d84f729d6 100644 --- a/pkg/cache/dbcache/util_test.go +++ b/pkg/cache/dbcache/util_test.go @@ -15,8 +15,6 @@ package dbcache import ( - "context" - "errors" "os/user" "sync" "time" @@ -24,8 +22,6 @@ import ( kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" porchapi "github.com/kptdev/porch/api/porch/v1alpha1" "github.com/kptdev/porch/pkg/repository" - mockrepo "github.com/kptdev/porch/test/mockery/mocks/porch/pkg/repository" - "github.com/stretchr/testify/mock" ) func (t *DbTestSuite) TestUtil() { @@ -129,36 +125,20 @@ func (t *DbTestSuite) TestGetOrInsertPkgLockAndDeletePkgLock() { deletePkgLock(pkgKey) } -func (t *DbTestSuite) TestExternalCommitInfo_NoGitLock() { - ctx := context.Background() - mockPR := mockrepo.NewMockPackageRevision(t.T()) - mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, nil).Once() - - commit, commitTime := externalCommitInfo(ctx, mockPR) - t.Equal("", commit) - t.True(commitTime.IsZero()) -} - -func (t *DbTestSuite) TestExternalCommitInfo_WithGitLock() { - ctx := context.Background() - mockPR := mockrepo.NewMockPackageRevision(t.T()) - mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{ - Git: &kptfilev1.GitLock{Commit: "abc123"}, - }, nil).Once() - - commit, commitTime := externalCommitInfo(ctx, mockPR) - t.Equal("abc123", commit) - t.True(commitTime.IsZero()) +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) TestExternalCommitInfo_GetLockError() { - ctx := context.Background() - mockPR := mockrepo.NewMockPackageRevision(t.T()) - mockPR.EXPECT().GetLock(mock.Anything).Return(kptfilev1.Upstream{}, kptfilev1.Locator{}, errors.New("lock error")).Once() - - commit, commitTime := externalCommitInfo(ctx, mockPR) - t.Equal("", commit) - t.True(commitTime.IsZero()) +func (t *DbTestSuite) TestHasBeenPushedToGit() { + now := time.Now() + t.False(hasBeenPushedToGit(&dbPackageRevision{})) + t.True(hasBeenPushedToGit(&dbPackageRevision{lastPushedDbUpdated: &now})) } func (t *DbTestSuite) TestDbContentChangedSincePush() { @@ -175,32 +155,9 @@ func (t *DbTestSuite) TestDbContentChangedSincePush() { t.True(dbContentChangedSincePush(pr)) } -func (t *DbTestSuite) TestExtCommitChangedSincePush() { - now := time.Now() - commit := "abc123" - otherCommit := "def456" - - pr := &dbPackageRevision{} - t.False(extCommitChangedSincePush(pr, "", now)) - - t.False(extCommitChangedSincePush(pr, commit, now)) - - pr.lastPushedCommit = &commit - t.False(extCommitChangedSincePush(pr, commit, now)) - - t.True(extCommitChangedSincePush(pr, otherCommit, now)) - - earlier := now.Add(-time.Second) - pr.lastPushedCommitTimestamp = &now - t.False(extCommitChangedSincePush(pr, otherCommit, earlier)) - - later := now.Add(time.Second) - t.True(extCommitChangedSincePush(pr, otherCommit, later)) -} - func (t *DbTestSuite) TestCommitTaskForPush() { - commit := "abc123" - pr := &dbPackageRevision{lastPushedCommit: &commit} + now := time.Now() + pr := &dbPackageRevision{lastPushedDbUpdated: &now} task := commitTaskForPush(pr) t.Require().NotNil(task) t.Equal(porchapi.TaskTypePush, task.Type) @@ -228,10 +185,6 @@ func (t *DbTestSuite) TestPrNeedsPushToGit() { pr := &dbPackageRevision{} t.True(prNeedsPushToGit(pr)) - commit := "abc123" - pr.lastPushedCommit = &commit - t.True(prNeedsPushToGit(pr)) - pr.updated = now pr.lastPushedDbUpdated = &now t.False(prNeedsPushToGit(pr)) @@ -242,29 +195,31 @@ func (t *DbTestSuite) TestPrNeedsPushToGit() { } func (t *DbTestSuite) TestPreservePushMarkersIfUnset() { - commit := "abc123" - commitTime := time.Now() - dbUpdated := commitTime.Add(-time.Minute) + dbUpdated := time.Now() existing := &dbPackageRevision{ - lastPushedCommit: &commit, - lastPushedCommitTimestamp: &commitTime, - lastPushedDbUpdated: &dbUpdated, + lastPushedDbUpdated: &dbUpdated, + extPRID: kptfilev1.Locator{ + Git: &kptfilev1.GitLock{Commit: "abc123"}, + }, } - pr := &dbPackageRevision{} + pr := &dbPackageRevision{ + extPRID: kptfilev1.Locator{ + Git: &kptfilev1.GitLock{Commit: unpushedGitCommit}, + }, + } preservePushMarkersIfUnset(pr, existing) - t.Require().NotNil(pr.lastPushedCommit) - t.Equal(commit, *pr.lastPushedCommit) - t.Equal(&commitTime, pr.lastPushedCommitTimestamp) - t.Equal(&dbUpdated, pr.lastPushedDbUpdated) + t.Require().NotNil(pr.lastPushedDbUpdated) + t.Equal(dbUpdated, *pr.lastPushedDbUpdated) + t.Equal("abc123", extPRCommit(pr)) - otherCommit := "other" - prWithMarker := &dbPackageRevision{lastPushedCommit: &otherCommit} + otherUpdated := dbUpdated.Add(time.Minute) + prWithMarker := &dbPackageRevision{lastPushedDbUpdated: &otherUpdated} preservePushMarkersIfUnset(prWithMarker, existing) - t.Equal(otherCommit, *prWithMarker.lastPushedCommit) + t.Equal(otherUpdated, *prWithMarker.lastPushedDbUpdated) prEmpty := &dbPackageRevision{} preservePushMarkersIfUnset(prEmpty, &dbPackageRevision{}) - t.Nil(prEmpty.lastPushedCommit) + t.Nil(prEmpty.lastPushedDbUpdated) } diff --git a/pkg/externalrepo/git/git.go b/pkg/externalrepo/git/git.go index 7446e836b..5374008e3 100644 --- a/pkg/externalrepo/git/git.go +++ b/pkg/externalrepo/git/git.go @@ -1957,32 +1957,15 @@ func (r *gitRepository) ClosePackageRevisionDraft(ctx context.Context, prd repos } } - // Read back the committer timestamp recorded on the commit so callers can persist the actual - // git-generated timestamp rather than a locally generated one. - var commitTime time.Time - if !commitHash.IsZero() { - if err := r.sharedDir.withLock(func(repo *git.Repository) error { - commitObj, err := repo.CommitObject(commitHash) - if err != nil { - return err - } - commitTime = commitObj.Committer.When - return nil - }); err != nil { - klog.Warningf("ClosePackageRevisionDraft: could not read commit %s to determine its timestamp: %v", commitHash, err) - } - } - return &gitPackageRevision{ - prKey: d.prKey, - repo: d.repo, - updated: updatedTime, - updatedBy: updatedBy, - ref: newRef, - tree: d.tree, - commit: commitHash, - commitTime: commitTime, - tasks: d.tasks, + prKey: d.prKey, + repo: d.repo, + updated: updatedTime, + updatedBy: updatedBy, + ref: newRef, + tree: d.tree, + commit: commitHash, + tasks: d.tasks, }, nil } diff --git a/pkg/externalrepo/git/package.go b/pkg/externalrepo/git/package.go index 74f07edfd..fad3ced63 100644 --- a/pkg/externalrepo/git/package.go +++ b/pkg/externalrepo/git/package.go @@ -41,7 +41,6 @@ type gitPackageRevision struct { ref *plumbing.Reference // ref is the Git reference at which the package exists tree plumbing.Hash // Cached tree of the package itself, some descendent of commit.Tree() commit plumbing.Hash // Current version of the package (commit sha) - commitTime time.Time // Committer timestamp recorded on commit (zero if unknown) tasks []porchapi.Task metadata metav1.ObjectMeta mutex sync.Mutex @@ -69,10 +68,6 @@ func (p *gitPackageRevision) Key() repository.PackageRevisionKey { return p.prKey } -func (p *gitPackageRevision) CommitTimestamp() time.Time { - return p.commitTime -} - func (p *gitPackageRevision) GetPackageRevision(ctx context.Context) (*porchapi.PackageRevision, error) { ctx, span := tracer.Start(ctx, "gitPackageRevision::GetPackageRevision", trace.WithAttributes()) defer span.End() diff --git a/pkg/externalrepo/git/package_tree.go b/pkg/externalrepo/git/package_tree.go index 629560335..4f17f8370 100644 --- a/pkg/externalrepo/git/package_tree.go +++ b/pkg/externalrepo/git/package_tree.go @@ -61,7 +61,6 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision var updated time.Time var updatedBy string - var commitTime time.Time // For the published packages on a tag or draft and proposed branches we know that the latest commit // if specific to the package in question. Thus, we can just take the last commit on the tag/branch. @@ -69,7 +68,6 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision if ref != nil && (isTagInLocalRepo(ref.Name()) || isDraftBranchNameInLocal(ref.Name()) || isProposedBranchNameInLocal(ref.Name())) { updated = p.parent.commit.Author.When updatedBy = p.parent.commit.Author.Email - commitTime = p.parent.commit.Committer.When } else { // If we are on the package branch, we can not assume that the last commit // pertains to the package in question. So we scan the git history to find @@ -83,7 +81,6 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision if commit != nil { updated = commit.Author.When updatedBy = commit.Author.Email - commitTime = commit.Committer.When } else { klog.Warningf("Cannot find latest package commit for package %s/%s: %s", p.pkgKey, revisionStr, err) } @@ -125,15 +122,14 @@ func (p *packageListEntry) buildGitPackageRevision(ctx context.Context, revision } return &gitPackageRevision{ - prKey: gitPrKey, - repo: repo, - updated: updated, - updatedBy: updatedBy, - ref: ref, - tree: p.treeHash, - commit: p.parent.commit.Hash, - commitTime: commitTime, - tasks: tasks, + prKey: gitPrKey, + repo: repo, + updated: updated, + updatedBy: updatedBy, + ref: ref, + tree: p.treeHash, + commit: p.parent.commit.Hash, + tasks: tasks, }, nil } diff --git a/pkg/repository/repository.go b/pkg/repository/repository.go index 111a3cc03..58dec9c69 100644 --- a/pkg/repository/repository.go +++ b/pkg/repository/repository.go @@ -306,12 +306,6 @@ type PackageRevision interface { IsLatestRevision() bool } -// CommitTimeGetter is optionally implemented by PackageRevision implementations that are backed by -// a git commit. CommitTimestamp returns the timestamp recorded on the underlying git commit -type CommitTimeGetter interface { - CommitTimestamp() time.Time -} - // Package is an abstract package. type Package interface { KubeObjectNamespace() string diff --git a/test/e2e/api/db_git_sync_test.go b/test/e2e/api/db_git_sync_test.go index d4cd248eb..bcedd55c5 100644 --- a/test/e2e/api/db_git_sync_test.go +++ b/test/e2e/api/db_git_sync_test.go @@ -55,6 +55,14 @@ func (t *PorchSuite) updatePRR(_ string, prr *porchapi.PackageRevisionResources, } } +func (t *PorchSuite) triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace string) string { + t.T().Helper() + branchName := suiteutils.DraftGitBranchName(packageName, workspace) + t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) + return branchName +} + func (t *PorchSuite) TestSyncDraftSurvivesSyncWhenInGit() { const ( repoName = dbGitTestRepoName + "-s1" @@ -267,7 +275,7 @@ data: "draft resources must contain concurrent update file %q; stale retry must not overwrite", newFileKey) } -func (t *PorchSuite) TestSyncPullsGitChangeIntoDB() { +func (t *PorchSuite) TestSyncDoesNotPullGitChangeIntoDB() { const ( repoName = dbGitTestRepoName + "-s6" packageName = "pkg-git-pull" @@ -282,12 +290,7 @@ func (t *PorchSuite) TestSyncPullsGitChangeIntoDB() { 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) - - time.Sleep(5 * time.Second) + branchName := t.triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace) // Commit a new file directly to the git branch, bypassing Porch. newFileContent := `apiVersion: v1 @@ -306,8 +309,8 @@ data: var prr porchapi.PackageRevisionResources t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) _, hasFromGit := prr.Spec.Resources[newFileKey] - t.Require().True(hasFromGit, - "package resources must contain %q after the external git commit was pulled into DB", 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() { @@ -385,10 +388,7 @@ func (t *PorchSuite) TestSyncReconcilesDBChangedAndPushesToGit() { 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) + branchName := t.triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace) initialSHA := t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName) t.Logf("initial branch commit SHA: %s", initialSHA) @@ -409,8 +409,7 @@ data: 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. reconcileBothPRs detects dbChanged && - // !extChanged (git is still at initialSHA) and enqueues a push. + // Unarchive and trigger sync. handleInBoth detects dbChanged and enqueues a push. t.SetGiteaRepoArchived(giteaRepo, false) t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) @@ -442,10 +441,7 @@ func (t *PorchSuite) TestSyncBothChangedDBWins() { 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) + branchName := t.triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace) initialSHA := t.GiteaGetBranchLatestCommitSHA(giteaRepo, branchName) t.Logf("initial branch commit SHA: %s", initialSHA) @@ -466,9 +462,8 @@ data: 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 while the - // repo is still archived. This advances the external commit past - // lastPushedCommitTimestamp, satisfying extChanged = true. + // 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, @@ -482,6 +477,16 @@ data: 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.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) t.WaitUntilGiteaBranchHasNewCommit(giteaRepo, branchName, externalOnlySHA, dbGitSyncWaitTimeout) From 85213145ccf3fa8d61295482a2551e229d7c3d7a Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Wed, 12 Aug 2026 10:40:06 +0200 Subject: [PATCH 07/13] Additional fixes Signed-off-by: Rendre Greyling --- pkg/cache/dbcache/dbpushtogit.go | 13 +++----- pkg/cache/dbcache/dbpushtogit_test.go | 22 ++++++++----- pkg/cache/dbcache/dbrepository.go | 6 ++++ pkg/cache/dbcache/util.go | 24 ++++++++++++-- pkg/cache/dbcache/util_test.go | 45 ++++++++++++++++++++++++--- pkg/externalrepo/git/package.go | 20 ++++++------ 6 files changed, 95 insertions(+), 35 deletions(-) diff --git a/pkg/cache/dbcache/dbpushtogit.go b/pkg/cache/dbcache/dbpushtogit.go index 6a4c172be..4157a99ce 100644 --- a/pkg/cache/dbcache/dbpushtogit.go +++ b/pkg/cache/dbcache/dbpushtogit.go @@ -60,11 +60,6 @@ func PushPublishedPackageRevision(ctx context.Context, repo repository.Repositor 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()) } - commitTask := &porchapi.Task{Type: porchapi.TaskTypePush} - if len(apiPr.Spec.Tasks) > 0 { - commitTask = &apiPr.Spec.Tasks[0] - } - var draft repository.PackageRevisionDraft var foundExisting bool @@ -82,7 +77,7 @@ func PushPublishedPackageRevision(ctx context.Context, repo repository.Repositor 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, commitTask); err != nil { + 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 @@ -95,7 +90,7 @@ func PushPublishedPackageRevision(ctx context.Context, repo repository.Repositor 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, commitTask); err != nil { + 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()) } } @@ -163,7 +158,7 @@ func PushDraftPackageRevision(ctx context.Context, repoKey repository.Repository updatedBeforePush := pr.updated - gitPRDraft, _, err := GetOrCreateGitDraft(ctx, pr.repo.externalRepo, pr) + 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 @@ -173,7 +168,7 @@ func PushDraftPackageRevision(ctx context.Context, repoKey repository.Repository Spec: porchapi.PackageRevisionResourcesSpec{ Resources: resources, }, - }, commitTaskForPush(pr)); err != nil { + }, commitTaskForPush(pr, existingGitPR != nil)); err != nil { klog.Warningf("PushDraftPackageRevision: repo %+v: UpdateResources failed for %+v: %v", repoKey, prKey, err) return } diff --git a/pkg/cache/dbcache/dbpushtogit_test.go b/pkg/cache/dbcache/dbpushtogit_test.go index a748fad90..91ac1363b 100644 --- a/pkg/cache/dbcache/dbpushtogit_test.go +++ b/pkg/cache/dbcache/dbpushtogit_test.go @@ -145,7 +145,7 @@ func TestPushPublishedPackageRevision_PushDraftsDisabled(t *testing.T) { tt.setupMocks(mockRepo, mockPR, mockPRD) - _, _, err := PushPublishedPackageRevision(ctx, mockRepo, mockPR, false, false) + _, err := PushPublishedPackageRevision(ctx, mockRepo, mockPR, false, false) if tt.expectError { assert.NotNil(t, err) } else { @@ -159,21 +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) - existingGitBranch bool - expectError bool + name string + setupMocks func(*mockrepo.MockRepository, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevision, *mockrepo.MockPackageRevisionDraft) + existingGitBranch bool + expectError bool }{ { 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.Anything).Return(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() @@ -253,7 +259,7 @@ func TestPushPublishedPackageRevision_PushDraftsEnabled(t *testing.T) { tt.setupMocks(mockRepo, mockPR, mockGitPR, mockPRD) - _, _, err := PushPublishedPackageRevision(ctx, mockRepo, mockPR, true, tt.existingGitBranch) + _, err := PushPublishedPackageRevision(ctx, mockRepo, mockPR, true, tt.existingGitBranch) if tt.expectError { assert.NotNil(t, err) } else { diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 3647f6d3f..14bc1cde1 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -351,6 +351,12 @@ func (r *dbRepository) UpdatePackageRevision(ctx context.Context, updatePR repos 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() diff --git a/pkg/cache/dbcache/util.go b/pkg/cache/dbcache/util.go index 6bcecca8e..d2934376e 100644 --- a/pkg/cache/dbcache/util.go +++ b/pkg/cache/dbcache/util.go @@ -108,7 +108,11 @@ func extPRCommitFromLocator(loc kptfile.Locator) string { } func hasBeenPushedToGit(pr *dbPackageRevision) bool { - return pr.lastPushedDbUpdated != nil + if pr.lastPushedDbUpdated != nil { + return true + } + commit := extPRCommit(pr) + return commit != "" && commit != unpushedGitCommit } func dbContentChangedSincePush(pr *dbPackageRevision) bool { @@ -118,8 +122,8 @@ func dbContentChangedSincePush(pr *dbPackageRevision) bool { return !pr.updated.Equal(*pr.lastPushedDbUpdated) } -func commitTaskForPush(pr *dbPackageRevision) *porchapi.Task { - if hasBeenPushedToGit(pr) { +func commitTaskForPush(pr *dbPackageRevision, existingInGit bool) *porchapi.Task { + if existingInGit || hasBeenPushedToGit(pr) { return &porchapi.Task{Type: porchapi.TaskTypePush} } @@ -132,6 +136,20 @@ func commitTaskForPush(pr *dbPackageRevision) *porchapi.Task { 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 diff --git a/pkg/cache/dbcache/util_test.go b/pkg/cache/dbcache/util_test.go index d84f729d6..3002ca129 100644 --- a/pkg/cache/dbcache/util_test.go +++ b/pkg/cache/dbcache/util_test.go @@ -139,6 +139,12 @@ 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() { @@ -158,25 +164,54 @@ func (t *DbTestSuite) TestDbContentChangedSincePush() { func (t *DbTestSuite) TestCommitTaskForPush() { now := time.Now() pr := &dbPackageRevision{lastPushedDbUpdated: &now} - task := commitTaskForPush(pr) + 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)) + t.Nil(commitTaskForPush(pr2, false)) pr3 := &dbPackageRevision{tasks: []porchapi.Task{{Type: porchapi.TaskTypeInit}}} - task3 := commitTaskForPush(pr3) + 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) + task4 := commitTaskForPush(pr4, false) t.Require().NotNil(task4) t.Equal(porchapi.TaskTypeClone, task4.Type) pr5 := &dbPackageRevision{} - t.Nil(commitTaskForPush(pr5)) + 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() { diff --git a/pkg/externalrepo/git/package.go b/pkg/externalrepo/git/package.go index fad3ced63..d27733de3 100644 --- a/pkg/externalrepo/git/package.go +++ b/pkg/externalrepo/git/package.go @@ -34,16 +34,16 @@ import ( ) type gitPackageRevision struct { - prKey repository.PackageRevisionKey - repo *gitRepository // repo is repo containing the package - updated time.Time - updatedBy string - ref *plumbing.Reference // ref is the Git reference at which the package exists - tree plumbing.Hash // Cached tree of the package itself, some descendent of commit.Tree() - commit plumbing.Hash // Current version of the package (commit sha) - tasks []porchapi.Task - metadata metav1.ObjectMeta - mutex sync.Mutex + prKey repository.PackageRevisionKey + repo *gitRepository // repo is repo containing the package + updated time.Time + updatedBy string + ref *plumbing.Reference // ref is the Git reference at which the package exists + tree plumbing.Hash // Cached tree of the package itself, some descendent of commit.Tree() + commit plumbing.Hash // Current version of the package (commit sha) + tasks []porchapi.Task + metadata metav1.ObjectMeta + mutex sync.Mutex } var _ repository.PackageRevision = &gitPackageRevision{} From f0d15deaf236d8ce4e8cdd4b2602965aa97a264e Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Wed, 12 Aug 2026 22:13:56 +0200 Subject: [PATCH 08/13] Fix Copilot reviews Signed-off-by: Rendre Greyling --- pkg/cache/dbcache/dbpackagerevision.go | 10 +-- pkg/cache/dbcache/dbpushtogit.go | 8 +- pkg/cache/dbcache/dbrepository.go | 29 +++---- pkg/cache/dbcache/dbreposync.go | 8 +- pkg/cache/dbcache/dbreposync_test.go | 13 +++- pkg/cache/dbcache/util.go | 61 ++++++++------- pkg/cache/dbcache/util_test.go | 77 ++++++++++--------- scripts/deploy/create-deployment-blueprint.sh | 2 +- test/e2e/crd/lifecycle_test.go | 4 +- test/e2e/suiteutils/gitea_test_utils.go | 6 +- 10 files changed, 112 insertions(+), 106 deletions(-) diff --git a/pkg/cache/dbcache/dbpackagerevision.go b/pkg/cache/dbcache/dbpackagerevision.go index 2c7815c24..cbc19747b 100644 --- a/pkg/cache/dbcache/dbpackagerevision.go +++ b/pkg/cache/dbcache/dbpackagerevision.go @@ -197,9 +197,8 @@ func (pr *dbPackageRevision) UpdateLifecycle(ctx context.Context, newLifecycle p _, span := tracer.Start(ctx, "dbPackageRevision::UpdateLifecycle", trace.WithAttributes()) defer span.End() - pkgMutex := getOrInsertPkgLock(pr.pkgRevKey.PkgKey) - pkgMutex.Lock() - defer pkgMutex.Unlock() + 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) @@ -394,9 +393,8 @@ func (pr *dbPackageRevision) SetMeta(ctx context.Context, meta metav1.ObjectMeta _, span := tracer.Start(ctx, "dbPackageRevision::SetMeta", trace.WithAttributes()) defer span.End() - pkgMutex := getOrInsertPkgLock(pr.pkgRevKey.PkgKey) - pkgMutex.Lock() - defer pkgMutex.Unlock() + lockPkgKey(pr.pkgRevKey.PkgKey) + defer unlockPkgKey(pr.pkgRevKey.PkgKey) pr.meta = meta diff --git a/pkg/cache/dbcache/dbpushtogit.go b/pkg/cache/dbcache/dbpushtogit.go index 4157a99ce..b149c987a 100644 --- a/pkg/cache/dbcache/dbpushtogit.go +++ b/pkg/cache/dbcache/dbpushtogit.go @@ -116,12 +116,8 @@ func PushDraftPackageRevision(ctx context.Context, repoKey repository.Repository prKey := pr.Key() klog.Infof("PushDraftPackageRevision: repo %+v started for %+v", repoKey, prKey) - pkgMutex := getOrInsertPkgLock(prKey.PKey()) - pkgMutex.Lock() - defer func() { - pkgMutex.Unlock() - deletePkgLock(prKey.PKey()) - }() + lockPkgKey(prKey.PKey()) + defer unlockPkgKey(prKey.PKey()) freshPR, err := pkgRevReadFromDB(ctx, prKey, true) if err != nil { diff --git a/pkg/cache/dbcache/dbrepository.go b/pkg/cache/dbcache/dbrepository.go index 14bc1cde1..017f33c02 100644 --- a/pkg/cache/dbcache/dbrepository.go +++ b/pkg/cache/dbcache/dbrepository.go @@ -287,12 +287,8 @@ func (r *dbRepository) DeletePackageRevision(ctx context.Context, pr2Delete repo Package: pr2Delete.Key().PKey().Package, } - pkgMutex := getOrInsertPkgLock(pk) - pkgMutex.Lock() - defer func() { - pkgMutex.Unlock() - deletePkgLock(pk) - }() + lockPkgKey(pk) + defer unlockPkgKey(pk) foundPkg, err := pkgReadFromDB(ctx, pk) if err != nil { @@ -343,9 +339,8 @@ func (r *dbRepository) UpdatePackageRevision(ctx context.Context, updatePR repos updatePkgRev.repo = r } - mutex := getOrInsertPkgLock(updatePkgRev.Key().PKey()) - mutex.Lock() - defer mutex.Unlock() + lockPkgKey(updatePkgRev.Key().PKey()) + defer unlockPkgKey(updatePkgRev.Key().PKey()) if err := updatePkgRev.UpdatePackageRevision(ctx); err != nil { return nil, err @@ -431,9 +426,8 @@ func (r *dbRepository) savePackageRevisionDraft(ctx context.Context, prd reposit d := prd.(*dbPackageRevision) - repoMutex := getOrInsertRepoLock(r.repoKey) - repoMutex.Lock() - defer repoMutex.Unlock() + lockRepoKey(r.repoKey) + defer unlockRepoKey(r.repoKey) return r.savePackageRevision(ctx, d, d.resourcesDirty) } @@ -442,9 +436,8 @@ func (r *dbRepository) savePackageRevision(ctx context.Context, d *dbPackageRevi _, span := tracer.Start(ctx, "dbRepository::savePackageRevision", trace.WithAttributes()) defer span.End() - pkgMutex := getOrInsertPkgLock(d.Key().PKey()) - pkgMutex.Lock() - defer pkgMutex.Unlock() + lockPkgKey(d.Key().PKey()) + defer unlockPkgKey(d.Key().PKey()) dbPkg, err := pkgReadFromDB(ctx, d.Key().PKey()) if err != nil { @@ -482,12 +475,12 @@ func (r *dbRepository) Refresh(ctx context.Context) error { _, span := tracer.Start(ctx, "dbRepository::Refresh", trace.WithAttributes()) defer span.End() - repoMutex := getOrInsertRepoLock(r.Key()) - repoMutex.Lock() - defer repoMutex.Unlock() + 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() diff --git a/pkg/cache/dbcache/dbreposync.go b/pkg/cache/dbcache/dbreposync.go index 7bf192967..44a611ed9 100644 --- a/pkg/cache/dbcache/dbreposync.go +++ b/pkg/cache/dbcache/dbreposync.go @@ -300,12 +300,8 @@ func (s *repositorySync) handleInCachedOnly(ctx context.Context, cachedPrMap map func (s *repositorySync) deleteCachedOnlyPR(ctx context.Context, dbPRKey repository.PackageRevisionKey, snapshot repository.PackageRevision) error { pkgKey := dbPRKey.PKey() - pkgMutex := getOrInsertPkgLock(pkgKey) - pkgMutex.Lock() - defer func() { - pkgMutex.Unlock() - deletePkgLock(pkgKey) - }() + lockPkgKey(pkgKey) + defer unlockPkgKey(pkgKey) freshPR, err := pkgRevReadFromDB(ctx, dbPRKey, false) if err != nil { diff --git a/pkg/cache/dbcache/dbreposync_test.go b/pkg/cache/dbcache/dbreposync_test.go index ba7a04d3e..69c2c0fd0 100644 --- a/pkg/cache/dbcache/dbreposync_test.go +++ b/pkg/cache/dbcache/dbreposync_test.go @@ -260,16 +260,23 @@ func (t *DbTestSuite) TestDBRepoSyncWithPushDraftsToGit_DraftOnlyInCacheQueuedFo 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 queue a git push instead of deleting it. // Explicitly trigger sync err = testRepo.repositorySync.SyncOnce(ctx) t.Require().NoError(err) - // Allow the async PushDraftPackageRevision goroutine to finish. - time.Sleep(200 * time.Millisecond) + 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) diff --git a/pkg/cache/dbcache/util.go b/pkg/cache/dbcache/util.go index d2934376e..f85328bb3 100644 --- a/pkg/cache/dbcache/util.go +++ b/pkg/cache/dbcache/util.go @@ -46,54 +46,63 @@ 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.RWMutex - locks map[string]*sync.Mutex + mu sync.Mutex + locks map[string]*keyedMutex } var globalLockManager = &lockManager{ - locks: make(map[string]*sync.Mutex), + locks: make(map[string]*keyedMutex), } -func (lm *lockManager) getLock(key string) *sync.Mutex { - lm.mu.RLock() - if m, exists := lm.locks[key]; exists { - lm.mu.RUnlock() - return m - } - lm.mu.RUnlock() - +func (lm *lockManager) lockKey(key string) { lm.mu.Lock() - defer lm.mu.Unlock() - if m, exists := lm.locks[key]; exists { - return m + km, exists := lm.locks[key] + if !exists { + km = &keyedMutex{} + lm.locks[key] = km } + km.refs++ + lm.mu.Unlock() - lm.locks[key] = new(sync.Mutex) - return lm.locks[key] + km.mu.Lock() } -func (lm *lockManager) deleteLock(key string) { +func (lm *lockManager) unlockKey(key string) { lm.mu.Lock() - defer lm.mu.Unlock() - delete(lm.locks, key) + 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 getOrInsertRepoLock(repoKey repository.RepositoryKey) *sync.Mutex { - return globalLockManager.getLock(repoKey.String()) +func unlockRepoKey(repoKey repository.RepositoryKey) { + globalLockManager.unlockKey(repoKey.String()) } -func getOrInsertPkgLock(pkgKey repository.PackageKey) *sync.Mutex { - return globalLockManager.getLock(pkgKey.String()) +func lockPkgKey(pkgKey repository.PackageKey) { + globalLockManager.lockKey(pkgKey.String()) } -func deletePkgLock(pkgKey repository.PackageKey) { - globalLockManager.deleteLock(pkgKey.String()) +func unlockPkgKey(pkgKey repository.PackageKey) { + globalLockManager.unlockKey(pkgKey.String()) } func extPRCommit(pr *dbPackageRevision) string { diff --git a/pkg/cache/dbcache/util_test.go b/pkg/cache/dbcache/util_test.go index 3002ca129..149597ac8 100644 --- a/pkg/cache/dbcache/util_test.go +++ b/pkg/cache/dbcache/util_test.go @@ -16,7 +16,6 @@ package dbcache import ( "os/user" - "sync" "time" kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1" @@ -70,59 +69,63 @@ func (t *DbTestSuite) TestSetValueFromJSONInvalidInput() { t.Equal(99, original) } -func (t *DbTestSuite) TestLockManagerGetLock() { - lm := &lockManager{locks: make(map[string]*sync.Mutex)} +func (t *DbTestSuite) TestLockManagerLockKey() { + lm := &lockManager{locks: make(map[string]*keyedMutex)} - lock1 := lm.getLock("key1") - t.NotNil(lock1) - - lock1Again := lm.getLock("key1") - t.Same(lock1, lock1Again) + lm.lockKey("key1") + km1 := lm.locks["key1"] + t.NotNil(km1) + t.Equal(1, km1.refs) + lm.unlockKey("key1") + t.Len(lm.locks, 0) - lock2 := lm.getLock("key2") - t.NotNil(lock2) - t.NotSame(lock1, lock2) + lm.lockKey("key2") + t.Len(lm.locks, 1) + lm.unlockKey("key2") } -func (t *DbTestSuite) TestLockManagerDeleteLock() { - lm := &lockManager{locks: make(map[string]*sync.Mutex)} +func (t *DbTestSuite) TestLockManagerWaitsOnSameMutex() { + lm := &lockManager{locks: make(map[string]*keyedMutex)} + const key = "concurrent-key" - lock := lm.getLock("mykey") - t.NotNil(lock) - t.Len(lm.locks, 1) + lm.lockKey(key) + km := lm.locks[key] - lm.deleteLock("mykey") - t.Len(lm.locks, 0) + secondAcquired := make(chan struct{}) + go func() { + lm.lockKey(key) + close(secondAcquired) + lm.unlockKey(key) + }() + + <-time.After(50 * time.Millisecond) + t.Same(km, lm.locks[key]) + t.Equal(2, km.refs) - lm.deleteLock("non-existent") + lm.unlockKey(key) + + <-secondAcquired + t.Len(lm.locks, 0) } -func (t *DbTestSuite) TestGetOrInsertRepoLock() { +func (t *DbTestSuite) TestLockRepoKey() { repoKey := repository.RepositoryKey{Namespace: "ns", Name: "repo"} - lock := getOrInsertRepoLock(repoKey) - t.NotNil(lock) - - lock2 := getOrInsertRepoLock(repoKey) - t.Same(lock, lock2) + lockRepoKey(repoKey) + t.Len(globalLockManager.locks, 1) + unlockRepoKey(repoKey) + t.Len(globalLockManager.locks, 0) } -func (t *DbTestSuite) TestGetOrInsertPkgLockAndDeletePkgLock() { +func (t *DbTestSuite) TestLockPkgKey() { pkgKey := repository.PackageKey{ RepoKey: repository.RepositoryKey{Namespace: "ns", Name: "repo"}, Package: "my-pkg", } - lock := getOrInsertPkgLock(pkgKey) - t.NotNil(lock) - - lock2 := getOrInsertPkgLock(pkgKey) - t.Same(lock, lock2) - - deletePkgLock(pkgKey) - lock3 := getOrInsertPkgLock(pkgKey) - t.NotNil(lock3) - - deletePkgLock(pkgKey) + lockPkgKey(pkgKey) + t.Len(globalLockManager.locks, 1) + unlockPkgKey(pkgKey) + t.Len(globalLockManager.locks, 0) } func (t *DbTestSuite) TestExtPRCommit() { diff --git a/scripts/deploy/create-deployment-blueprint.sh b/scripts/deploy/create-deployment-blueprint.sh index 3887254c1..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 draft push flags for porch-server and porch-controllers + --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 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 ef1b53ed3..02275ace9 100644 --- a/test/e2e/suiteutils/gitea_test_utils.go +++ b/test/e2e/suiteutils/gitea_test_utils.go @@ -157,7 +157,11 @@ func (t *TestSuite) CreateGiteaRepoNoCleanup(repoName string) string { t.T().Helper() body := fmt.Sprintf(`{"name":%q,"auto_init":true,"readme":"Default"}`, repoName) - req, _ := http.NewRequest("POST", t.GetGiteaApiURL()+"/api/v1/user/repos", strings.NewReader(body)) + 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") From 8276cb1d7ffc4f5b99e2bd384821a34c2fe42e42 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Thu, 27 Aug 2026 11:31:45 +0200 Subject: [PATCH 09/13] Update pkg/cache/dbcache/dbpushtogit.go Co-authored-by: Fiachra Corcoran Signed-off-by: Rendre Greyling --- pkg/cache/dbcache/dbpushtogit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cache/dbcache/dbpushtogit.go b/pkg/cache/dbcache/dbpushtogit.go index b149c987a..643aca920 100644 --- a/pkg/cache/dbcache/dbpushtogit.go +++ b/pkg/cache/dbcache/dbpushtogit.go @@ -1,4 +1,4 @@ -// Copyright 2025 The kpt Authors +// 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. From 0292981de7dc18069da53c8f692fcd451e9ab049 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Mon, 31 Aug 2026 14:53:14 +0200 Subject: [PATCH 10/13] Fix lint issue Signed-off-by: Rendre Greyling --- test/e2e/suiteutils/suite_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index 50c15dd52..b7dfb6639 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -816,7 +816,7 @@ func (t *TestSuite) TriggerRepoSync(repoName string, timeout time.Duration) { // 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 = ptr.To(runOnceAt) + repo.Spec.Sync.RunOnceAt = new(runOnceAt) t.UpdateF(&repo) t.Logf("TriggerRepoSync: set runOnceAt for repo %s, waiting for sync to complete", repoName) From b18dbdf60aa1dfda8b05264ee356194cc817be7a Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Tue, 11 Aug 2026 15:26:44 +0200 Subject: [PATCH 11/13] Fix e2e tests Signed-off-by: Rendre Greyling --- scripts/deploy/create-deployment-blueprint.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/deploy/create-deployment-blueprint.sh b/scripts/deploy/create-deployment-blueprint.sh index 967005fbf..3887254c1 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 and repo controller + --db-push-drafts-to-git BOOL ... enable draft push flags for porch-server and porch-controllers --create-v1alpha2-rpkg BOOL ... enable v1alpha2 PackageRevision CRD creation by repo controller EOF exit 1 From d53e826e42624a425eb24bc905073720ae828748 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Wed, 12 Aug 2026 22:13:56 +0200 Subject: [PATCH 12/13] Fix Copilot reviews Signed-off-by: Rendre Greyling --- scripts/deploy/create-deployment-blueprint.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/deploy/create-deployment-blueprint.sh b/scripts/deploy/create-deployment-blueprint.sh index 3887254c1..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 draft push flags for porch-server and porch-controllers + --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 From 49eee5da587347aa9ea497dd088241c098836847 Mon Sep 17 00:00:00 2001 From: Rendre Greyling Date: Mon, 7 Sep 2026 13:09:34 +0200 Subject: [PATCH 13/13] Fix failing e2e Signed-off-by: Rendre Greyling --- pkg/cache/dbcache/util_test.go | 6 +++--- test/e2e/api/db_git_sync_test.go | 21 ++++++++++---------- test/e2e/suiteutils/suite_utils.go | 31 ++++++++++++++++++++++-------- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/pkg/cache/dbcache/util_test.go b/pkg/cache/dbcache/util_test.go index 149597ac8..54d6a681c 100644 --- a/pkg/cache/dbcache/util_test.go +++ b/pkg/cache/dbcache/util_test.go @@ -98,9 +98,9 @@ func (t *DbTestSuite) TestLockManagerWaitsOnSameMutex() { lm.unlockKey(key) }() - <-time.After(50 * time.Millisecond) - t.Same(km, lm.locks[key]) - t.Equal(2, km.refs) + t.Eventually(func() bool { + return lm.locks[key] == km && km.refs == 2 + }, time.Second, 5*time.Millisecond) lm.unlockKey(key) diff --git a/test/e2e/api/db_git_sync_test.go b/test/e2e/api/db_git_sync_test.go index bcedd55c5..6a45499d3 100644 --- a/test/e2e/api/db_git_sync_test.go +++ b/test/e2e/api/db_git_sync_test.go @@ -27,7 +27,7 @@ import ( const ( dbGitTestRepoName = "db-git-test-repo" - dbGitSyncWaitTimeout = 60 * time.Second + dbGitSyncWaitTimeout = 90 * time.Second ) func (t *PorchSuite) updatePRR(_ string, prr *porchapi.PackageRevisionResources, resourceKeys ...string) { @@ -58,11 +58,17 @@ func (t *PorchSuite) updatePRR(_ string, prr *porchapi.PackageRevisionResources, func (t *PorchSuite) triggerRepoSyncAndWaitForDraftBranch(repoName, giteaRepo, packageName, workspace string) string { t.T().Helper() branchName := suiteutils.DraftGitBranchName(packageName, workspace) - t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) + 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" @@ -160,8 +166,7 @@ data: ` t.updatePRR(repoName, &prr, "recovery.yaml") - t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) - + t.RequestRepoSync(repoName) t.WaitUntilGiteaBranchExists(giteaRepo, branchName, dbGitSyncWaitTimeout) t.Logf("draft branch %s recovered into git after the failed push", branchName) @@ -411,9 +416,7 @@ data: // Unarchive and trigger sync. handleInBoth detects dbChanged and enqueues a push. t.SetGiteaRepoArchived(giteaRepo, false) - t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) - - newSHA := t.WaitUntilGiteaBranchHasNewCommit(giteaRepo, branchName, initialSHA, dbGitSyncWaitTimeout) + 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) @@ -487,9 +490,7 @@ data: ` t.updatePRR(repoName, &prr, dbFileKey) - t.TriggerRepoSync(repoName, dbGitSyncWaitTimeout) - - t.WaitUntilGiteaBranchHasNewCommit(giteaRepo, branchName, externalOnlySHA, dbGitSyncWaitTimeout) + t.triggerRepoSyncAndWaitForNewCommit(repoName, giteaRepo, branchName, externalOnlySHA) t.GetF(client.ObjectKey{Namespace: t.Namespace, Name: pr.Name}, &prr) _, hasDBFile := prr.Spec.Resources[dbFileKey] diff --git a/test/e2e/suiteutils/suite_utils.go b/test/e2e/suiteutils/suite_utils.go index b7dfb6639..e7ade2272 100644 --- a/test/e2e/suiteutils/suite_utils.go +++ b/test/e2e/suiteutils/suite_utils.go @@ -798,18 +798,15 @@ func (t *TestSuite) GetPackageRevisionWithFilter(repo, pkgName string, filter Pa return &prList.Items[0] } -func (t *TestSuite) TriggerRepoSync(repoName string, timeout time.Duration) { +// 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) - baselineLastSync := time.Time{} - if repo.Status.LastFullSyncTime != nil { - baselineLastSync = repo.Status.LastFullSyncTime.Time - } - if repo.Spec.Sync == nil { repo.Spec.Sync = &configapi.RepositorySync{} } @@ -819,8 +816,26 @@ func (t *TestSuite) TriggerRepoSync(repoName string, timeout time.Duration) { repo.Spec.Sync.RunOnceAt = new(runOnceAt) t.UpdateF(&repo) - t.Logf("TriggerRepoSync: set runOnceAt for repo %s, waiting for sync to complete", repoName) - t.WaitForNextRepoSync(repoName, timeout, baselineLastSync, runOnceAt.Time) + 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) {