Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/src/test/scala/zipx/docs/AffectedDoc.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 =
Expand Down
4 changes: 3 additions & 1 deletion docs/src/test/scala/zipx/docs/Verify.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 20 additions & 20 deletions modules/core/src/main/scala/zipx/core/Planner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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],
Expand All @@ -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),
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))

Expand Down
16 changes: 16 additions & 0 deletions modules/core/src/test/scala/zipx/core/AffectedDeploySpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
36 changes: 32 additions & 4 deletions modules/core/src/test/scala/zipx/core/AffectedPublishSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
39 changes: 39 additions & 0 deletions modules/core/src/test/scala/zipx/core/MatrixCollapseSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
44 changes: 44 additions & 0 deletions modules/core/src/test/scala/zipx/core/PlannerSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down