Skip to content

Do not report notifications from guard skipped resources - #55

Open
tas50 wants to merge 1 commit into
chef:mainfrom
tas50:fix/guarded-notifications
Open

Do not report notifications from guard skipped resources#55
tas50 wants to merge 1 commit into
chef:mainfrom
tas50:fix/guarded-notifications

Conversation

@tas50

@tas50 tas50 commented Aug 22, 2026

Copy link
Copy Markdown

Fixes chefspec#863
Fixes chefspec#751

Problem

Notifications stay declared on a resource even when a guard stopped its action from running, and the notify matcher reads that declaration directly. So a resource whose only_if never passed is still reported as notifying:

execute 'reload' do
  command 'true'
  action :nothing
end

file '/tmp/guarded' do
  action :delete
  notifies :run, 'execute[reload]', :immediately
  only_if { false }
end
expected "file[/tmp/guarded]" to not notify "execute[reload]", but it did.

This is an internal contradiction rather than a design tradeoff. In the same run ChefSpec correctly reports that the action was skipped and that the notified resource never ran:

is_expected.to_not delete_file('/tmp/guarded')  # passes
is_expected.to_not run_execute('reload')        # passes

Fix

The obvious fix, suppressing notifications when performed_actions is empty, is wrong. Chef's ConditionalActionNotNothing makes should_skip? true for action :nothing too, so a guard skipped resource and a notified action :nothing resource look identical. That naive version breaks legitimate chained notifications and fails the existing notifications::chained and subscribes examples.

Instead, record genuine guard skips separately in run_action and only suppress notifications when the resource ran nothing and a guard refused it.

Compatibility

This is the one behavior change in the set. A test that asserts a guarded resource notifies passes today and will start failing. That assertion was asserting something Chef does not do, but it is still visible breakage, so it is worth a changelog note.

Testing

Adds a notifications::guarded acceptance example covering a guarded resource and an unguarded one for comparison.

  • With the fix: 14 examples, 0 failures
  • With the fix reverted: 14 examples, 1 failure
  • notifications, subscribes and guards examples all pass
  • Unit suite: 197 examples, 0 failures

@tas50
tas50 requested review from a team and jaymzh as code owners August 22, 2026 00:07
Notifications stay declared on a resource even when a guard stopped its
action from running, and the notify matcher read that declaration
directly. A resource whose only_if never passed was therefore reported as
notifying, even though ChefSpec correctly reported that its action was
skipped and that the notified resource never ran.

Tracking this needs care because Chef's ConditionalActionNotNothing makes
should_skip? true for an :nothing action as well, so a resource that is
notified and then notifies onward looks identical to a guard skipped one.
Record genuine guard skips separately and only suppress notifications
when the resource ran nothing and a guard refused it.

Fixes chefspec#863
Fixes chefspec#751

Signed-off-by: Tim Smith <tsmith84@proton.me>
@tas50
tas50 force-pushed the fix/guarded-notifications branch from 03d3f55 to 9c27f93 Compare August 22, 2026 00:09
@tas50

tas50 commented Aug 22, 2026

Copy link
Copy Markdown
Author

CI on this PR is red for an unrelated reason: main's bundle cannot be installed on Linux, which breaks every job before any test runs. That is fixed separately in #56, which is green across Ruby 3.1 to 3.4. This PR should go green once #56 lands and this branch is rebased.

Locally, against a working bundle, this branch passes rake unit and the acceptance examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

incorrect result when testing conditional notification Notifications to resources bypass guards set on the resource

1 participant