Defining branch_pattern sends notifications from tag workflows

[brandon-leapyear]

Orb version:

4.3.3

What happened:

we have a step defined like:

- slack/notify:
    branch_pattern: master
    event: fail
    template: basic_fail_1
    channel: nightly-builds-notifications

where we want ONLY workflows kicked off by running the master branch to send notifications. What happened was we had a workflow kicked off from a git tag and the notification got sent from that workflow.

It seems like this behavior comes from the first line in FilterBy:

if [[ -z "$1" ]] || [[ -z "$2" ]];
  return
fi

Expected behavior:

If I define a branch_pattern, it should ONLY send the notification if the workflow was kicked off by a branch AND the branch matches the pattern. Likewise, if I define a tag_pattern, it should ONLY send the notification if the workflow was kicked off by a git tag AND the tag matches the pattern.

I'd expect behavior more like

 FilterBy() {
-    if [ -z "$1" ] || [ -z "$2" ]; then
-      return
-    fi
 
     ...
 }
 
 ShouldPost() {
     if [ "$CCI_STATUS" = "$SLACK_PARAM_EVENT" ] || [ "$SLACK_PARAM_EVENT" = "always" ]; then
         # In the event the Slack notification would be sent, first ensure it is allowed to trigger
         # on this branch or this tag.
-        FilterBy "$SLACK_PARAM_BRANCHPATTERN" "${CIRCLE_BRANCH:-}"
-        FilterBy "$SLACK_PARAM_TAGPATTERN" "${CIRCLE_TAG:-}"
+        if [[ -n "${CIRCLE_BRANCH:-}" ]]; then
+            FilterBy "$SLACK_PARAM_BRANCHPATTERN" "$CIRCLE_BRANCH"
+        elif [[ -n "${CIRCLE_TAG:-}" ]]; then
+            FilterBy "$SLACK_PARAM_TAGPATTERN" "$CIRCLE_TAG"
+        else
+            # deduplicate logic in else block
+            exit 0
+        fi
 
         echo "Posting Status"
     else
         ...
     fi
 }

Additional Information:

[bobbyrenwick]

This is happening to us but in reverse, we're not setting a branch_pattern, just a tag_pattern and that's causing notifications when the build is triggered by a branch

[woylie]

Same here. We'd like to have success notifications only for production releases, but with a configuration like this:

- slack/notify:
          event: pass
          channel: ...
          template: success_tagged_deploy_1
          tag_pattern: prod\/.+

We also get success notifications for deployments triggered by merging to our stage branch (without a tag).

[EricRibeiro]

Hi folks 👋

Sorry for taking so long to get to this. @brandon-leapyear, I looked at #273, and I believe it doesn't solve the problem. Take @woylie's scenario:

Same here. We'd like to have success notifications only for production releases, but with a configuration like this:

- slack/notify:
          event: pass
          channel: ...
          template: success_tagged_deploy_1
          tag_pattern: prod\/.+

We also get success notifications for deployments triggered by merging to our stage branch (without a tag).

CIRCLE_BRANCH won't be empty, as they are in the staging branch, and SLACK_PARAM_BRANCHPATTERN equals .+, its default value. Therefore it falls in ShouldPost's first conditional (ref), and it doesn't abort, leading to an unwanted notification:

    if [ -n "${CIRCLE_BRANCH:-}" ]; then
        if ! FilterBy "$SLACK_PARAM_BRANCHPATTERN" "$CIRCLE_BRANCH"; then
            AbortPost \
                "Branch pattern does not match: ${SLACK_PARAM_BRANCHPATTERN}" \
                "CI was triggered by branch: ${CIRCLE_BRANCH}"
        fi

I can see a few solutions, but none are as good as writing an unmatchable pattern for the subject you don't want triggering workflows. Going back to @woylie's scenario, I would have the config to be:

- slack/notify:
          event: pass
          channel: ...
          template: success_tagged_deploy_1
          tag_pattern: prod\/.+
          branch_pattern: x\by # https://codegolf.stackexchange.com/questions/18393/shortest-unmatchable-regular-expression

Alternatives that I can think of involve creating extra parameters or breaking changes. None of which is justifiable when there is a relatively simple solution at hand. I'll close the issue, but I'll be happy to revisit it if a better solution presents itself.