Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track status state per build step instead of per pipeline #107

Closed
wants to merge 3 commits into from

Conversation

yasunariw
Copy link
Collaborator

@yasunariw yasunariw commented Jan 7, 2021

Description of the task

Because two builds on the same pipeline + branch don't necessarily always run the same build steps, the current state tracking is insufficient for handling those cases. Instead of tracking build state per pipeline, we should track it per build step.

Some details regarding Buildkite notification behavior:

The implementation:

For each status notification that passes the rule check (which should only be overall build notifications), we:

  1. Retrieve all status notifications associated with this commit using GH's API
  2. Filter the list to only get the status notifications that belong to the same build (in case of rebuilds)
  3. Filter the list again for the most recent status notification of each build step (which should either be success or failure)
  4. Update the runtime state, which maps pipeline/build step names to per-branch build states.
  5. Just like before, this is then queried whenever there is an "allow_once" match. An "allow_once" match will generate a notification if any of the build steps associated with the current build have a different status value from the previous build.

Future work:

It would be nice if build failure notifications also told us which build step failed. This can be done easily by making Action.partition_status returned the build steps that failed, so that Slack.generate_status_notification can include them in the final message.

Final note:

Large diff size is due to the addition of HTTP request/response stubs - apologies.

How to test

Existing tests should pass.

Two cases are added; the second handles the behavior observed in production that motivated this PR.

status.success_test_different_steps_from_prev

This is for an incoming successful build on develop branch with a single build step, "notabot-test/build". There exist past successes for same branch + different steps, and for different branch + same step, but not for same branch + same step. Thus, it should generate a notification.

status.success_test_not_affected_by_unrelated_success_with_different_steps

This is for an incoming successful build on develop branch with two steps, "notabot-test/{build-infra,setup}". Here, previously a build with steps "build-infra" and "setup" failed on step "build-infra". Then a subsequent build with step "build" succeeded, so the overall pipeline state is a success. However, the more recent successful but unrelated build should not affect generation of another success notification, given the change in status state for "build-infra".

make test

References

`status.success_test_different_steps_from_prev`

This is for an incoming successful build on develop branch with
a single build step, "notabot-test/build". There exist past successes
for same branch + different steps, and for different branch + same
step, but not for same branch + same step. Thus, it should generate
a notification.

`status.success_test_not_affected_by_unrelated_success_with_different_steps`

This is for an incoming successful build on develop branch with
two steps, "notabot-test/{build-infra,setup}". Here, previously a build with
steps "build-infra" and "setup" failed on step "build-infra". Then a
subsequent build with step "build" succeeded, so the overall pipeline state
is a success. However, the more recent successful but unrelated build should not
affect generation of another success notification, given the change in status
state for "build-infra".
@yasunariw yasunariw force-pushed the yasu/track-status-per-build-step branch from 9373fb5 to 091b1c2 Compare January 8, 2021 09:05
@yasunariw yasunariw requested review from ygrek and Khady January 8, 2021 09:12
@yasunariw yasunariw marked this pull request as ready for review January 8, 2021 09:15
@yasunariw yasunariw force-pushed the yasu/track-status-per-build-step branch from 091b1c2 to 7978002 Compare November 30, 2021 06:29
@Khady
Copy link
Contributor

Khady commented Mar 20, 2024

I wonder what would be the implication of this one now that we have direct messages

@Khady Khady added the Blocked label Sep 30, 2024
@Khady
Copy link
Contributor

Khady commented Sep 30, 2024

@thatportugueseguy can we close this PR now?

@thatportugueseguy
Copy link
Collaborator

yes 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants