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.

Sign up for GitHub

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

Jump to bottom

[mergify] report open backported PRs once a week #28964

Merged
merged 2 commits into from
Nov 19, 2021
Merged

[mergify] report open backported PRs once a week #28964

merged 2 commits into from
Nov 19, 2021

Conversation

v1v
Copy link
Member

@v1v v1v commented Nov 15, 2021 •
edited
Loading

What does this PR do?

Notify to the assignees when an automated backport Pull Request has not been merged yet.

When?:

  • Once a week (modays)
  • PR was created by mergify
  • It has at least one GitHub check. (We want to notify regardless of the GitHub check status)
  • It has at least one assignee.

Why is it important?

Avoid missing backports.

UI

See elastic/apm-pipeline-library#1380 (comment)

image

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 15, 2021
@v1v v1v requested review from ruflin, jsoriano and a team November 15, 2021 13:06
@mergify
Copy link
Contributor

mergify bot commented Nov 15, 2021

This pull request does not have a backport label. Could you fix it @v1v? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@v1v v1v self-assigned this Nov 15, 2021
@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Nov 15, 2021
@v1v v1v added automation Team:Automation Label for the Observability productivity team labels Nov 15, 2021
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 15, 2021
ruflin
ruflin approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I initially was thinking it puts a reminder after it is stale for a week but I can see how it makes the implementation much more complicated. Lets go with the pings each Monday.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 15, 2021 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-18T19:09:42.136+0000

  • Duration: 15 min 30 sec

  • Commit: 4c5b072

Test stats 🧪

Test Results
Failed 0
Passed 3
Skipped 0
Total 3

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

This was referenced Nov 15, 2021
Merged
Merged
jsoriano
jsoriano approved these changes Nov 15, 2021
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 added some comments and questions, nothing blocker.

.mergify.yml Show resolved Hide resolved
.mergify.yml
actions:
comment:
message: |
This pull request has not been merged yet. Could you please review and merge it @{{ assignee | join(', @') }}? 🙏
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some backports don't have an assignee (for example #28783), should we have a different action for these ones? (Not sure which one)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create a new one to add a team by default if no assignee?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I don't know who should be pinged on this action. We can leave this for a follow up.

The only thing to consider here is that assignee may be empty, do we still want to add a comment in this case? It will generate a message like:

This pull request has not been merged yet. Could you please review and merge it @? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reported anymore to those PRs without an assignee. We will work on this in the future with another rule to notify to a more broadly team, maybe...

.mergify.yml Show resolved Hide resolved
This was referenced Nov 15, 2021
Merged
Merged
Merged
jsoriano
jsoriano approved these changes Nov 16, 2021
View reviewed changes
.mergify.yml Show resolved Hide resolved
.mergify.yml Show resolved Hide resolved
.mergify.yml
actions:
comment:
message: |
This pull request has not been merged yet. Could you please review and merge it @{{ assignee | join(', @') }}? 🙏
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I don't know who should be pinged on this action. We can leave this for a follow up.

The only thing to consider here is that assignee may be empty, do we still want to add a comment in this case? It will generate a message like:

This pull request has not been merged yet. Could you please review and merge it @? 🙏

v1v
v1v commented Nov 16, 2021
View reviewed changes
.mergify.yml Show resolved Hide resolved
@v1v v1v merged commit 3d88a44 into elastic:master Nov 19, 2021
@v1v v1v deleted the feature/mergify-notify-missing-backports branch November 19, 2021 10:49
v1v added a commit to v1v/beats that referenced this pull request Nov 22, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/support-a…
bb20e3d 
…ws-on-file-changes

* upstream/master:
  Fix discovery of Nomad allocations (elastic#28700)
  Add null (`\u0000`) as a valid line terminator (elastic#28998)
  Remove `logging.files.suffix` option and always use datetime suffixes (elastic#28927)
  x-pack/filebeat/module: add note for default var.input (elastic#28324)
  Fix AccessList & AccessMask processing in security data_stream (elastic#29016)
  [Metricbeat] Fix wrong mapping on "info" subkey (elastic#28782)
  ci: daily/weekly jobs (elastic#29050)
  [mergify] report open backported PRs once a week (elastic#28964)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@cachedout cachedout cachedout approved these changes

@jsoriano jsoriano jsoriano approved these changes

Assignees

@v1v v1v

Labels
automation backport-skip Skip notification from the automated backport with mergify Team:Automation Label for the Observability productivity team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@v1v @elasticmachine @jsoriano @cachedout @ruflin