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

workflow: generate PR comments when non-required tests fail and let the action itself succeed #1016

Merged

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Nov 6, 2024

Prologue

A while ago I attempted to do this thing, made a mess, and gave up.

Now, I am happy to report that with a little distance and time, I realised I was being, as @mvo5 would say, a Muppet.

This PR changes the osbuild-composer integration check workflow.

Current workflow

Currently, we have a test that checks if the HEAD of a given PR breaks osbuild-composer compatibility. This test is not required for merging, but is meant to serve as a warning that compatibility with osbuild-composer is either already broken or will be and that some work will be required to update the dependency. This is a good thing to know and kudos to @thozza for introducing it.

However, there's a couple of things about the current workflow that proved to be a bit annoying in the long run:

  1. When osbuild-composer compatibility is already broken in main, every PR will be marked as failing the check. This makes sense, the HEAD of the PR will fail to work with osbuild-composer, which is what we're testing for. But it's also not very useful information for someone who is opening a PR and knows they're not changing the API or anything that could affect the integration.
  2. Breaking osbuild-composer compatibility is a frequent occurrence. Maybe that's a problem in and of itself, but we kinda know what we're doing when we do it. This means that very frequently we have PRs open that are "good to merge" but show a red X as their status in lists and overviews.

New workflow

The new workflow doesn't fail if the integration test fails. Instead it posts a message notifying the author and reviewers of a PR that compatibility/integration with osbuild-composer will fail when the dependency is updated. This message is only posted on a PR that causes the incompatibility issue. If compatibility is already failing on the merge base of the PR (i.e. on main), the notice is not posted.
As an extra bonus, if a message is posted due to breakage, and the same PR is updated to fix the issue, the comment is edited to reflect that it has been fixed.

The comment in the workflow file itself explains how things work in more detail.

Message wording

Suggestions about the wording or any extra information you'd like to see in the comments are more than welcome.

Example repo

While working on this, I created a clone of this repository to experiment with everything I wanted to do and get the workflow just right. In the repo there are three open PRs:

  1. Nothing: The PR does not affect the integration with osbuild-composer and nothing happens.
  2. Break the API: The PR renamed a public enum value used by osbuild-composer to break the integration. The notice is posted.
  3. Break then fix the API: Started the same as 2, breaking the API, and waited until the message was posted. Then the commit was reverted in a new commit and the comment on the PR changes to say the integration has been fixed.

@achilleas-k achilleas-k added the github_actions Pull requests that update GitHub Actions code label Nov 6, 2024
@achilleas-k achilleas-k force-pushed the github/non-required-test-comments branch from 284aeba to 7e946db Compare November 6, 2024 19:22
intergation -> integration
@achilleas-k achilleas-k force-pushed the github/non-required-test-comments branch from 7e946db to e806411 Compare November 7, 2024 11:54
The new workflow doesn't fail if the integration test fails.  Instead it
posts a message notifying the author and reviewers of a PR that
compatibility/integration with osbuild-composer will fail when the
dependency is updated.

This message is only posted on a PR that causes the incompatibility
issue.  If compatibility is already failing on the merge base of the PR
(i.e. on main), the notice is not posted.

The comment in the workflow explains how things work in more detail.
@achilleas-k achilleas-k force-pushed the github/non-required-test-comments branch from e806411 to c68dfa8 Compare November 7, 2024 17:23
@achilleas-k
Copy link
Member Author

I tested two more scenarios on the fork:

@achilleas-k achilleas-k added this pull request to the merge queue Nov 8, 2024
Merged via the queue into osbuild:main with commit d73bfcc Nov 8, 2024
17 checks passed
@achilleas-k achilleas-k deleted the github/non-required-test-comments branch November 8, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants