Add skip_on_no_changes to keep unmatched steps' depends_on resolvable

[Megh03]

When a monorepo-diff watch path doesn't match any changed file, the step is silently omitted from the generated pipeline. If another step's depends_on points at that omitted step's key, the reference never resolves, so, the build stalls or fails waiting on a step that was never created.

This adds an opt-in skip_on_no_changes flag: when enabled, unmatched watch steps are still emitted, but marked skip: instead of omitted, so depends_on references to them resolve correctly. A skipped step in Buildkite counts as completed, unblocking anything depending on it.

Resolves SUP-6863.
Resolves #172.

- monorepo-diff#v1.x.x:
    skip_on_no_changes: true
    watch:
      - path: services/
        config:
          group: CI/CD Infrastructure
          key: group:cicd
          steps:
            - command: echo deploy
      - path: app/
        config:
          command: echo build-app
          depends_on: group:cicd

If services/ doesn't match any changed file, the group is now emitted with skip: "No changes detected" instead of vanishing, and build-app's depends_on resolves.

[omehegan]

This all looks good! Great to see the expansion in unit test coverage. The functional change makes sense to me, no concerns about that. I would just suggest that you add a README update to document the new function. Feel free to merge once that's done!

[petetomasik]

except_path-excluded watches are still fully omitted here, even when skip_on_no_changes: true, since the continue happens before the skip-placeholder logic below ever runs. If another step's depends_on points at this watch's key, the build stalls the same way as the bug this PR fixes, just triggered by except_path instead of an unmatched path.

[petetomasik]

appendStep's collision logic has three branches for two watches sharing a Key, but the tests only cover two orderings: real match first then skip placeholder (dropped), and both unmatched (more specific reason wins). This branch, a skip placeholder appended first, then a later watch with the same key producing a real match that supersedes it in place, isn't exercised anywhere. Perhaps it'd be worth a test case to lock it in (maybe a 3+-watch collision too), since a future edit to this switch could silently invert it with nothing to catch it.

[petetomasik]

Passing skip_on_no_changes through here means stepsToTrigger can now return skip-placeholder steps even when nothing truly matched, which makes hasSteps true below (line 112) and triggers a pipeline upload that previously wouldn't have happened. If every watch is unmatched and there's no default step, every build now uploads a pipeline containing only skip: steps, instead of skipping the upload entirely.

Probably the right trade-off given the feature's purpose, but it's a real behavior change with no test at this level and nothing documenting it. Worth a test (or at least a note) confirming that's intended?

[petetomasik]

Following on from my earlier comment on this line: the test you added covers the upload-happens-anyway case, thanks. There's one consequence of it that I think is worth a line in the README, because it's the part a user would be surprised by rather than the upload itself.

Pipeline-level notify is attached to the generated pipeline map rather than to yamlSteps, so it doesn't contribute to the hasSteps decision. That means a config with notify but no wait and no hooks used to short-circuit before the upload when nothing matched, and now doesn't:

• flag off, nothing matched: hasSteps=false, no upload, no notification
• flag on, nothing matched: hasSteps=true, upload happens, notifications fire

So someone who opts in and has a plugin-level Slack notify starts getting a ping on every build where nothing matched. Configs that set wait or hooks were already always uploading, since those get appended to yamlSteps before the len(yamlSteps) == 0 check, so this is the narrow real delta rather than a broad one.

I think it's defensible given what the feature is for, but it's a surprising interaction between two unrelated options and it'd be cheap to call out in the skip_on_no_changes section.

[petetomasik]

defaultSteps gets appended with a raw append here, which bypasses appendStep and so bypasses the keyIndex invariant that a key never lands in the output twice. dedupSteps can't clean it up either, because the placeholder and the default step differ in Skip, so reflect.DeepEqual is false.

Reproduced with watch: [{path: "foo/", config: {key: k, command: run}}, {default: {key: k, command: fallback}}] and a changed file of bar/x.txt:

# skip_on_no_changes: true
steps:
  - command: run
    skip: No changes detected
    key: k
  - command: fallback
    key: k

With the flag off, and on main, that same config emits a single step (key: k, command: fallback). Buildkite rejects an upload containing duplicate keys, so the build fails to start rather than degrading. Sharing a key between a real watch and its default fallback is the natural way to keep a stable depends_on target, which is exactly the use case this feature exists to serve, so I don't think this is an unlikely config.

for _, s := range defaultSteps { appendStep(s) } should cover it.

There's a second hole in the same invariant worth fixing alongside: keyIndex only tracks top-level Step.Key, not keys on steps nested inside a group. A keyless group: container with keyed nested steps, duplicated across two watch entries where one path matches and one doesn't, emits both the real group and the placeholder group and duplicates the nested key:

steps:
  - group: Deploy
    steps:
      - command: echo deploy
        key: deploy-job
  - group: Deploy
    steps:
      - command: echo deploy
        key: deploy-job
    skip: No changes detected

On main the unmatched watch vanished, so there was no duplicate to hit.

That also makes the comment at the bottom of the appendStep switch inaccurate: it says a duplicate key is surfaced by Buildkite "same as it always has for any other duplicate-key mistake", but in both of these cases the duplicate is new behaviour introduced here, not a pre-existing misconfiguration. Worth rewording once the two paths are fixed, since that comment is what a future reader will trust.

[petetomasik]

This changes behaviour for people who never turn skip_on_no_changes on.

anyMatched is set whenever a watch's path matches, regardless of whether that watch actually contributed any steps, and it then replaces the old len(steps) == 0 guard on the default fallback below. A watch that declares a path but no config matches, produces zero steps, and now suppresses the default watch that used to fire.

Reproduced with watch: [{path: "app/"}, {default: {command: echo default}}] and a changed file of app/main.go, with skip_on_no_changes left off:

• origin/main: one step, command: echo default
• this branch: zero steps

A watch with no config is a sloppy config, but the plugin tolerates it today and degrades to "the default runs". Losing the default silently means a build that previously ran a fallback step now runs nothing, which is the kind of thing that gets noticed a week later. It also cuts against the "(legacy behaviour)" test cases, which are asserting that the flag-off path is untouched.

The old guard was really "did any watch contribute a real step", so I think the fix is to only count matches that produced something, rather than matches:

if matched && len(w.Steps) > 0 {
	anyMatched = true
}

or track the count of non-placeholder appends and keep the condition as realSteps == 0, which stays closer to the original semantics.

Either way it'd be good to have a test for the stepless-watch-plus-default combination with the flag both off and on, since nothing in the suite covers it right now on either side.

[petetomasik]

"A skipped step in Buildkite counts as completed, so any depends_on reference to it still resolves" is the load-bearing claim for this whole feature, and I can't find anything that confirms it for the skip attribute specifically.

Every Buildkite source I checked scopes the "skipped dependencies are satisfied" behaviour to if-conditional skipping and never to skip:. The depends_on page reads "When a step is skipped (due to an if condition returning false), any steps that depend on that step will still run", and the state table row is literally "Skipped (due to if condition)". The conditionals page then muddies it further by saying a job whose if returns false becomes broken, which it calls "distinct from skipped jobs", so the one documented case may not even be the job state we're producing here.

The circumstantial evidence points the right way: skip is a legal attribute on command, trigger and group steps, and its docs say skipped steps are "hidden in the pipeline view by default, but can be made visible by toggling the 'Skipped jobs' icon", which implies a real terminal job exists in the build graph. But that's inference, not confirmation.

What the tests currently prove is that the emitted YAML is schema-valid. They can't prove the dependency actually resolves at runtime, so if this premise is wrong the feature is inert and nothing in the suite would tell us. Given we're closing SUP-6863 and #172 on it, I'd like that verified against a real build before this merges.

.buildkite/pipeline.yml already has :bomb:/:testtube: steps covering triggers, groups, hooks, wait, notifications and both default config forms, so an e2e step here would follow the existing pattern. e2e/one-match-one-miss looks like it fits more or less as-is: one watch with a group key that matches, one that doesn't, and a third step with depends_on pointing at the unmatched key. If the build gets past that step, the premise holds.

One thing to add to this section either way: because group-level skip is merged down into each step inside the group rather than applied to the group itself, an all-skipped group still renders as a visible but apparently-empty group container. Worth setting that expectation here, since the example in this section is a group and people will wonder why it's still showing up.

[petetomasik]

Following on from my earlier comment on this line: the test you added covers the upload-happens-anyway case, thanks. There's one consequence of it that I think is worth a line in the README, because it's the part a user would be surprised by rather than the upload itself.

Pipeline-level notify is attached to the generated pipeline map rather than to yamlSteps, so it doesn't contribute to the hasSteps decision. That means a config with notify but no wait and no hooks used to short-circuit before the upload when nothing matched, and now doesn't:

• flag off, nothing matched: hasSteps=false, no upload, no notification
• flag on, nothing matched: hasSteps=true, upload happens, notifications fire

So someone who opts in and has a plugin-level Slack notify starts getting a ping on every build where nothing matched. Configs that set wait or hooks were already always uploading, since those get appended to yamlSteps before the len(yamlSteps) == 0 check, so this is the narrow real delta rather than a broad one.

I think it's defensible given what the feature is for, but it's a surprising interaction between two unrelated options and it'd be cheap to call out in the skip_on_no_changes section.