diff --git a/docs/src/test/scala/zipx/docs/AffectedDoc.scala b/docs/src/test/scala/zipx/docs/AffectedDoc.scala index 79998a1..d2291c8 100644 --- a/docs/src/test/scala/zipx/docs/AffectedDoc.scala +++ b/docs/src/test/scala/zipx/docs/AffectedDoc.scala @@ -188,8 +188,8 @@ Three properties carry over unchanged, and each is what makes the opt-in safe ra 3. **A skipped image never silently skips its deploy.** This is the trap the feature opens, so the planner closes it (see the next section). -The `affected` job itself now runs on tag pushes when a Publish capability reads it, where a Verify-only setup skips -there. That is free: on a tag it takes no diff. +The `affected` job itself now runs on tag pushes and on merged-PR pushes when a Publish or Deploy capability reads +it, where a Verify-only setup skips there. That is free: on a non-PR event it takes no diff, and it emits `["all"]`. """, exampleValue { given PlanConfig = config.copy(affected = AffectedMode.AffectedOnPR, affectedPublish = true) @@ -315,8 +315,8 @@ that module's artifacts really are unchanged. The cost of `Graph` over `Aggregat target), and with it one approval per module per environment. Both safety properties carry over. A **release tag deploys everything**: the `affected` job is forced onto tag -pushes when a Deploy capability reads it, and there it emits `["all"]` without taking a diff. And an unusable diff -**fails open** to `["all"]` the same way. +pushes and merged-PR pushes when a Deploy capability reads it, and on a non-PR event it emits `["all"]` without +taking a diff. And an unusable diff **fails open** to `["all"]` the same way. """, exampleValue { given PlanConfig = diff --git a/docs/src/test/scala/zipx/docs/Verify.scala b/docs/src/test/scala/zipx/docs/Verify.scala index b84d1d0..739fd71 100644 --- a/docs/src/test/scala/zipx/docs/Verify.scala +++ b/docs/src/test/scala/zipx/docs/Verify.scala @@ -155,7 +155,9 @@ Changed files → owning module (longest base-dir prefix) → reverse-dependency section("Skip Verify after merge / on tags")( md""" By default (`zipxSkipMergedPrPush := true`), a push to `main` that lands a merged PR does **not** re-run Verify. -Direct pushes still Verify. **Tag pushes never run Verify** (release tags only need Publish / Deploy). +Direct pushes still Verify. **Tag pushes never run Verify** (release tags only need Publish / Deploy). Graph +Publish and Deploy still run after a merge; the `affected` job they read stays up and emits `["all"]` on that +non-PR push, so they do not `fromJson` an empty output. With **LocalDir**, that skip would otherwise leave `main` without an `actions/cache` save (PR caches are branch-scoped; later PRs only warm from the default branch). So by default zipx also emits a minimal diff --git a/modules/core/src/main/scala/zipx/core/Planner.scala b/modules/core/src/main/scala/zipx/core/Planner.scala index a436e8c..4f3900c 100644 --- a/modules/core/src/main/scala/zipx/core/Planner.scala +++ b/modules/core/src/main/scala/zipx/core/Planner.scala @@ -353,13 +353,14 @@ object Planner: config.affected == AffectedMode.AffectedOnPR && capabilities.exists(c => affectedGatedPhase(c.phase, config) && c.scope == CapabilityScope.Graph) - // Publish and Deploy jobs run on a release tag, where Verify does not, so the `affected` job they now depend on has - // to run there too. It emits the `all` sentinel for a tag push already (see `affectedScript`), which is what makes a - // release publish and deploy everything regardless of any diff. + // Publish and Deploy jobs run on a release tag and on a merged-PR push, where Verify does not, so the `affected` + // job they depend on has to run there too. It emits the `all` sentinel for a non-PR event already (see + // `affectedScript`), which is what makes a release publish and deploy everything regardless of any diff. // // Both phases, not just Publish: a tag-gated Graph deploy would otherwise carry `needs: affected` and an expression - // reading its output on a ref where that job does not exist. - val affectedOnTags = + // reading its output on a ref where that job does not exist. The same hole on a merged-PR push is `fromJson("")`: + // GitHub's skipped-job output is empty, not `'[]'`. + val affectedWhenVerifySkips = usesAffected && List(Phase.Publish, Phase.Deploy).exists(phase => affectedGatedPhase(phase, config) && capabilities.exists(c => c.phase == phase && c.scope == CapabilityScope.Graph @@ -421,7 +422,8 @@ object Planner: Option.when(usesVerifyGate)(verifyGateJobId -> verifyGateJob(config)), Option.when(usesCacheRehydrate)(cacheRehydrateJobId -> cacheRehydrateJob(config)), Option.when(usesAffected)( - affectedJobId -> affectedSetupJob(config, usesVerifyGate, affectedOnTags) + affectedJobId -> + affectedSetupJob(config, usesVerifyGate && !affectedWhenVerifySkips, affectedWhenVerifySkips) ), ).flatten @@ -570,8 +572,8 @@ object Planner: * run is for a docs-only deploy). Non-Verify phases pass through untouched. * * @param excludeTagsAndDispatch - * `false` keeps the merged-PR skip but drops the tag/dispatch exclusion, for the one job that is Verify-shaped and - * yet has to run on a tag: the `affected` setup job, once Publish reads its output too. + * `false` drops the tag/dispatch exclusion. Combined with `usesVerifyGate = false`, this is how the `affected` + * setup job stays running on a tag and on a merged-PR push once Publish or Deploy reads its output. */ private def applyVerifyGate( needs: List[JobId], @@ -598,14 +600,14 @@ object Planner: (gatedNeeds, andConditions(Some(gateCond.unwrapped), cond)) end if - /** @param runsOnTags - * an affected-gated Publish job runs on a release tag, so the job it reads its module list from has to as well. - * Cheap: on a tag push [[affectedScript]] takes no diff at all, it emits the `all` sentinel directly, which is - * what makes a release publish everything. + /** @param runsWhenVerifySkips + * an affected-gated Publish or Deploy job runs on a release tag and on a merged-PR push, where Verify does not, so + * the job it reads its module list from has to as well. Cheap: on a non-PR event [[affectedScript]] takes no diff + * at all, it emits the `all` sentinel directly, which is what makes a release publish everything. */ - private def affectedSetupJob(config: PlanConfig, usesVerifyGate: Boolean, runsOnTags: Boolean): Job = + private def affectedSetupJob(config: PlanConfig, usesVerifyGate: Boolean, runsWhenVerifySkips: Boolean): Job = val (needs, cond) = - applyVerifyGate(Nil, None, Phase.Verify, usesVerifyGate, excludeTagsAndDispatch = !runsOnTags) + applyVerifyGate(Nil, None, Phase.Verify, usesVerifyGate, excludeTagsAndDispatch = !runsWhenVerifySkips) Job( name = Some("affected"), runsOn = List(config.runnerOs), @@ -1153,8 +1155,7 @@ object Planner: val clauses = tolerance.headOption.toList ++ releaseGate.toList ++ affectedGate.toList ++ tolerance.drop(1) val baseCond = if clauses.isEmpty then None else Some(clauses.mkString(" && ")) val (needs, gated) = - if gatedOnAffected then applyVerifyGate(rawNeeds, baseCond, capability.phase, usesVerifyGate = false) - else applyVerifyGate(rawNeeds, baseCond, capability.phase, usesVerifyGate) + applyVerifyGate(rawNeeds, baseCond, capability.phase, usesVerifyGate) val targetCond = targets.headOption.flatMap(t => JobCondition.renderOpt(t.condition)) val cond = andConditions(andConditions(gated, JobCondition.renderOpt(capability.condition)), targetCond) @@ -1255,11 +1256,10 @@ object Planner: val guardedNeeds = rawNeeds.filterNot(id => id == affectedJobId || id == verifyGateJobId) val skipTolerant = dependsOnSkippable(capability, affectedGatedNames) val baseCond = jobCondition(capability, node, guardedNeeds, gatedOnAffected, skipTolerant) - // The affected setup job already needs verify-gate, so a gated-on-affected job inherits the skip through it and asks - // only for the tag exclusion. + // Verify jobs carry their own gate. They must not inherit a merged-PR skip by hoping `affected` is skipped: when + // Publish or Deploy reads `affected`, that job stays running so later `fromJson` sees real JSON. val (needs, gated) = - if gatedOnAffected then applyVerifyGate(rawNeeds, baseCond, capability.phase, usesVerifyGate = false) - else applyVerifyGate(rawNeeds, baseCond, capability.phase, usesVerifyGate) + applyVerifyGate(rawNeeds, baseCond, capability.phase, usesVerifyGate) val cond = andConditions(gated, JobCondition.renderOpt(capability.condition)) val runner = capability.runsOn.getOrElse(List(config.runnerOs)) diff --git a/modules/core/src/test/scala/zipx/core/AffectedDeploySpec.scala b/modules/core/src/test/scala/zipx/core/AffectedDeploySpec.scala index 2437183..7151bde 100644 --- a/modules/core/src/test/scala/zipx/core/AffectedDeploySpec.scala +++ b/modules/core/src/test/scala/zipx/core/AffectedDeploySpec.scala @@ -294,6 +294,22 @@ object AffectedDeploySpec extends ZIOSpecDefault: ) assertTrue(cond(wf, "affected").contains("!startsWith(github.ref, 'refs/tags/')")) }, + test("the affected job runs on a merged-PR push once Deploy reads it") { + val wf = plan( + List( + Capability.testGraph.withMatrixCollapse(MatrixCollapse.Off), + dockerExpanded, + deployGraph(), + ), + on.copy(skipMergedPrPush = true), + ) + assertTrue( + !cond(wf, "affected").contains("needs.verify-gate.outputs.run == 'true'"), + cond(wf, "test-serviceA").contains("needs.verify-gate.outputs.run == 'true'"), + !wf.jobs("deploy-serviceA-prod").needs.contains("verify-gate"), + cond(wf, "deploy-serviceA-prod").contains("needs.affected.outputs.modules"), + ) + }, test("fail-open is unchanged: an unusable diff deploys everything") { assertTrue( Affected.outputModules(dockerGraphFixture, None) == Affected.AllSentinel, diff --git a/modules/core/src/test/scala/zipx/core/AffectedPublishSpec.scala b/modules/core/src/test/scala/zipx/core/AffectedPublishSpec.scala index 7ea7a90..6c3bb80 100644 --- a/modules/core/src/test/scala/zipx/core/AffectedPublishSpec.scala +++ b/modules/core/src/test/scala/zipx/core/AffectedPublishSpec.scala @@ -188,13 +188,41 @@ object AffectedPublishSpec extends ZIOSpecDefault: cond(wf, "test-schema").contains("!startsWith(github.ref, 'refs/tags/')"), ) }, - test("the merged-PR skip still applies to the affected job, only the tag exclusion is dropped") { + test("the affected job runs on a merged-PR push once Publish reads it") { + // Without this the whole publish is skipped: `affected` would carry Verify's merged-PR skip, skip after + // merge, and every Publish job's membership test would `fromJson` an empty output. val wf = plan(List(testExpanded, publishExpanded), on.copy(skipMergedPrPush = true)) assertTrue( - wf.jobs("affected").needs.contains("verify-gate"), - // Fail-open: a gate that did not succeed (on a tag it is skipped outright) lets this run anyway. - cond(wf, "affected").contains("needs.verify-gate.result != 'success'"), + !cond(wf, "affected").contains("needs.verify-gate.outputs.run == 'true'"), !cond(wf, "affected").contains("!startsWith(github.ref, 'refs/tags/')"), + cond(wf, "test-schema").contains("needs.verify-gate.outputs.run == 'true'"), + !wf.jobs("publish-schema").needs.contains("verify-gate"), + ) + }, + test("with only Verify gated, the affected job still skips after a merged PR") { + val wf = plan(List(testExpanded), off.copy(skipMergedPrPush = true)) + assertTrue( + cond(wf, "affected").contains("needs.verify-gate.outputs.run == 'true'"), + cond(wf, "test-schema").contains("needs.verify-gate.outputs.run == 'true'"), + ) + }, + test("affected emits JSON whenever a later job will fromJson it") { + val wf = plan(List(testExpanded, publishExpanded), on.copy(skipMergedPrPush = true)) + val fromJson = "fromJson(needs.affected.outputs.modules)" + val consumers = wf.jobs.filter { (id, job) => + id != "affected" && ( + job.`if`.exists(_.contains(fromJson)) || + job.steps.exists(_.`if`.exists(_.contains(fromJson))) + ) + } + val mergedPrSkip = "needs.verify-gate.outputs.run == 'true'" + assertTrue( + consumers.nonEmpty, + consumers.forall((_, job) => job.needs.contains("affected")), + // affected's skip condition is a subset of the consumers': the merged-PR skip is not on affected, so + // it cannot skip while a consumer's if: is still true. + !cond(wf, "affected").contains(mergedPrSkip), + consumers.exists((_, job) => job.`if`.exists(_.contains("needs.affected.outputs.modules"))), ) }, test("fail-open is unchanged: an unusable diff publishes everything") { diff --git a/modules/core/src/test/scala/zipx/core/MatrixCollapseSpec.scala b/modules/core/src/test/scala/zipx/core/MatrixCollapseSpec.scala index 088d64e..c7d7d5f 100644 --- a/modules/core/src/test/scala/zipx/core/MatrixCollapseSpec.scala +++ b/modules/core/src/test/scala/zipx/core/MatrixCollapseSpec.scala @@ -446,6 +446,45 @@ object MatrixCollapseSpec extends ZIOSpecDefault: !api.`if`.exists(_.contains("matrix.")), ) }, + test("collapsed Graph Publish cannot fromJson a skipped affected job") { + // Production shape: job-level `modules != '[]'`, step-level `contains(fromJson(...), matrix.module)`. + // GitHub's skipped-job output is `""`, not `'[]'`, so `!= '[]'` does not protect a step `fromJson`. + val cfg = baseConfig.copy( + affected = AffectedMode.AffectedOnPR, + affectedPublish = true, + skipMergedPrPush = true, + ) + val image = Capability + .custom( + name = CapabilityName("image"), + command = n => SbtCommand.module(n, SbtCommand.unsafeTask("Docker/publishLocal")), + participates = _.docker, + phase = Phase.Verify, + gate = Gate.Always, + ordering = Ordering.ParallelWithUpstream, + ) + .withMatrixCollapse(MatrixCollapse.Coarse) + val wf = Planner.plan(independentDocker, List(image, dockerCap(MatrixCollapse.Coarse)), cfg) + val affectedIf = wf.jobs("affected").`if`.getOrElse("") + val docker = wf.jobs("docker") + val imageJob = wf.jobs("image") + val fromJson = "fromJson(needs.affected.outputs.modules)" + val dockerFromJson = + docker.`if`.exists(_.contains(fromJson)) || docker.steps.exists(_.`if`.exists(_.contains(fromJson))) + val imageFromJson = + imageJob.`if`.exists(_.contains(fromJson)) || + imageJob.steps.exists(_.`if`.exists(_.contains(fromJson))) + assertTrue( + !affectedIf.contains("needs.verify-gate.outputs.run == 'true'"), + docker.`if`.exists(_.contains("needs.affected.outputs.modules != '[]'")), + dockerFromJson, + docker.needs.contains("affected"), + !docker.needs.contains("verify-gate"), + imageJob.`if`.exists(_.contains("needs.verify-gate.outputs.run == 'true'")), + imageJob.needs.contains("verify-gate"), + imageFromJson, + ) + }, test("rendered YAML job if line is matrix-free for every collapsing mode") { check(Gen.elements(MatrixCollapse.Auto, MatrixCollapse.Strict, MatrixCollapse.Coarse)) { mode => val cfg = baseConfig.copy(affected = AffectedMode.AffectedOnPR, affectedPublish = true) diff --git a/modules/core/src/test/scala/zipx/core/PlannerSpec.scala b/modules/core/src/test/scala/zipx/core/PlannerSpec.scala index e93453e..097a8b0 100644 --- a/modules/core/src/test/scala/zipx/core/PlannerSpec.scala +++ b/modules/core/src/test/scala/zipx/core/PlannerSpec.scala @@ -1151,6 +1151,50 @@ object PlannerSpec extends ZIOSpecDefault: !wf.jobs("publish").`if`.exists(_.contains("!startsWith(github.ref, 'refs/tags/')")), ) }, + test("skipMergedPrPush gates Graph Verify even when it is affected-gated") { + // Once `affected` itself no longer skips after a merged PR (Publish/Deploy read it), Graph Verify must + // carry its own gate clause. Skip-inheritance-through-affected is how production `fromJson("")` happened. + val graph = sampleGraph.mapNodes { + case n if n.id.startsWith("service") => n.copy(docker = true) + case n => n + } + val image = Capability + .custom( + name = CapabilityName("image"), + command = n => SbtCommand.module(n, SbtCommand.unsafeTask("Docker/publishLocal")), + participates = _.docker, + phase = Phase.Verify, + gate = Gate.Always, + ordering = Ordering.ParallelWithUpstream, + ) + .withMatrixCollapse(MatrixCollapse.Coarse) + val docker = Capability.dockerGraph.withMatrixCollapse(MatrixCollapse.Coarse) + val wf = Planner.plan( + graph, + List(Capability.test, image, docker), + config.copy( + skipMergedPrPush = true, + affected = AffectedMode.AffectedOnPR, + affectedPublish = true, + ), + ) + val fromJson = "fromJson(needs.affected.outputs.modules)" + assertTrue( + wf.jobs("test").`if`.exists(_.contains("needs.verify-gate.outputs.run == 'true'")), + wf.jobs("image").`if`.exists(_.contains("needs.verify-gate.outputs.run == 'true'")), + wf.jobs("image").needs.contains("verify-gate"), + wf.jobs("image").needs.contains("affected"), + !wf.jobs("affected").`if`.exists(_.contains("needs.verify-gate.outputs.run == 'true'")), + !wf.jobs("docker").needs.contains("verify-gate"), + wf.jobs("docker").`if`.exists(_.contains("needs.affected.outputs.modules")), + wf.jobs("docker").steps.exists(_.`if`.exists(_.contains(fromJson))), + wf.jobs("cache-rehydrate") + .`if` + .contains( + "needs.verify-gate.result == 'success' && needs.verify-gate.outputs.run == 'false'" + ), + ) + }, test("skipMergedPrPush false omits verify-gate but still skips Verify on tags and dispatch") { val wf = Planner.plan(sampleGraph, List(Capability.test), config.copy(skipMergedPrPush = false)) assertTrue(