Add backport reminder and reorganize the backporting actions.#889
Add backport reminder and reorganize the backporting actions.#889NickDris merged 37 commits intoelastic:masterfrom
Conversation
|
This needs a bit of optimization, in order to apply the DRY principle, as the functionality added in #888 has some significant overlaps. |
* Add a backport reminder for unbackported commits * Add direnv to .gitignore * Simplify logic and comments * Fixed linting issue * Unify label and reminder logic to one module
There was a problem hiding this comment.
Initial high-level feedback/questions:
- Having a CLI tool which can be used in GH actions as well as independently is a great idea.
- A dry-run option is highly recommended. It would allow for testing (part of PR review).
- Token should not be passed through command-line option, see below.
- The tool uses standard libraries only it seems. Is this intentional? Example: why
argparseand not something newer, likeclick? backport.action.ymlstill referencesscripts/backport-pending.pywhich gets removed.
|
All of the review comments were much help. |
This reverts commit 901c119.
gbanasiak
left a comment
There was a problem hiding this comment.
Many thanks for these exhaustive tests. Personally I do find some of them too complex to follow easily. One thing that really adds to the complexity is the attempt to verify entire sequences of GH API calls. I think I would keep this simpler looking for key API call depending on the test case. But that's fine, let's not spend more time simplifying these.
I'd like to request the following 2 changes:
- change filtering logic for reminders - they should apply to PRs with
backport pendinglabel - make newly added tests discoverable by
pytest(andhatch -v -e unit run test), they are currently not covered; the problem is all.*folders are by default excluded frompytestdue to defaultnorecursedirssetting (doc) - my suggestion would be to rename.citotoolsand removescriptssub-dir (tools/scriptsseems redundant now, we only have 2 scripts); we also need an extra__init__.pyfile undertoolsto makepytesthappy
Reminder comments will accumulate in time, especially in case of PRs waiting for the first supported release. It would be nice to remove older comments after adding a new one.
There's a bunch of static type checker (mypy) failures. I've started mentioning them in review but gave up quickly. I recommend using type checker integrated with IDE for now.
My heart sank for a moment when looking at dry-run outputs. I haven't noticed this earlier :)
# these print-outs do not mention dry-run and "added" suggests it was already done
% ./tools/scripts/backport.py --dry-run --repo=elastic/rally-tracks label --lookback-days=7
2025-11-18 17:21:29,111 INFO __main__ Added label 'backport pending' to PR #917
2025-11-18 17:21:29,112 INFO __main__ Added label 'backport pending' to PR #916
2025-11-18 17:21:29,112 INFO __main__ Added label 'backport pending' to PR #915
2025-11-18 17:21:29,112 INFO __main__ Added label 'backport pending' to PR #904
# this output sometimes says "would" which is nice but mixes this with "posted", confusing
% ./tools/scripts/backport.py --dry-run --repo=elastic/rally-tracks remind --lookback-days=7
2025-11-18 17:27:36,473 INFO __main__ prefetched=12 lookback_days=7 pending_reminder_age_days=7 now=2025-11-18T16:27:36.473747+00:00 threshold=2025-11-11T16:27:36.473747+00:00
2025-11-18 17:27:36,802 INFO __main__ Would post comment to PR #918
2025-11-18 17:27:36,802 INFO __main__ PR #918: initial reminder posted
2025-11-18 17:27:36,802 INFO __main__ PR #917: cooling period not elapsed)
2025-11-18 17:27:36,802 INFO __main__ PR #916: cooling period not elapsed)
2025-11-18 17:27:36,802 INFO __main__ PR #915: cooling period not elapsed)
2025-11-18 17:27:37,146 INFO __main__ Would post comment to PR #911
2025-11-18 17:27:37,146 INFO __main__ PR #911: initial reminder posted
2025-11-18 17:27:37,491 INFO __main__ Would post comment to PR #910
2025-11-18 17:27:37,491 INFO __main__ PR #910: initial reminder posted
2025-11-18 17:27:37,810 INFO __main__ Would post comment to PR #909
2025-11-18 17:27:37,811 INFO __main__ PR #909: initial reminder posted
2025-11-18 17:27:38,212 INFO __main__ Would post comment to PR #908
2025-11-18 17:27:38,212 INFO __main__ PR #908: initial reminder posted
2025-11-18 17:27:38,656 INFO __main__ Would post comment to PR #907
2025-11-18 17:27:38,657 INFO __main__ PR #907: initial reminder posted
2025-11-18 17:27:38,657 INFO __main__ PR #904: cooling period not elapsed)
2025-11-18 17:27:39,065 INFO __main__ Would post comment to PR #902
2025-11-18 17:27:39,065 INFO __main__ PR #902: initial reminder posted
2025-11-18 17:27:39,455 INFO __main__ Would post comment to PR #898
2025-11-18 17:27:39,455 INFO __main__ PR #898: initial reminder posted
There was a problem hiding this comment.
I have the filing that this PR got lot of unnecessary complexity and technical debt just because we didn't reused pre-existing 3rd party code [1]. It is nice to write code to learn and get code reviewed to be able to exchange ideas and experiences. But for something like this I would have preferred using a pre-existing library. It would have been much less work than writing (and testing) this code. Given the quality of the code of this PR is not very critical to me it LGTM but please read my comments so you can do different the next time (I hope I am not been rude, it has never been my intention).
[1] https://github.com/PyGithub/PyGithub
Extra context: We've been there before with discussion around |
|
@elasticmachine update branch |
|
merge conflict between base and head |
Nothing rude about it. I do have the same expectations from myself in the future but it's a learning process and you are actually helping it. Thank you . |
gbanasiak
left a comment
There was a problem hiding this comment.
LGTM, I've left final cosmetic changes after testing
Co-authored-by: Grzegorz Banasiak <grzegorz.banasiak@elastic.co>
…c#889) Refactored backport cli to include all requirements existing at the moment for automated backporting of PRs. Supporting label and remind commands and --dry-run for testing.
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
…c#889) Refactored backport cli to include all requirements existing at the moment for automated backporting of PRs. Supporting label and remind commands and --dry-run for testing.
Refactored backport cli to include all requirements existing at the moment for automated backporting of PRs. Supporting label and remind commands and --dry-run for testing.
Refactored backport cli to include all requirements existing at the moment for automated backporting of PRs. Supporting label and remind commands and --dry-run for testing.
…c#889) Refactored backport cli to include all requirements existing at the moment for automated backporting of PRs. Supporting label and remind commands and --dry-run for testing.
…c#889) Refactored backport cli to include all requirements existing at the moment for automated backporting of PRs. Supporting label and remind commands and --dry-run for testing.
* Tests tracks selectively based on PR changes (#858) * CI builds ES revision from sources based on es-version file (#875) * Drop Python 3.9 and introduce Python 3.13 in CI (#877) * [ES-13188] Update GH macOS hosted runner image (#883) * Address pytest deprecations (#911) * CI determines Elasticsearch build arguments using unified job in gh workflow [stateful] (#925) * Fix an error where ci arguments were not applied properly (#928) * Fix an error where ci arguments were not applied properly * Test the change * Test failed retry with another fix * Previous fix failed retry * Revert es-version to current * Keep the indents if if/else * Add backport reminder and reorganize the backporting actions. (#889) * Rename IT folders (#938) * Fix pytest skip argument (#905) * Backport reminders - fix missing default values in workflow (#947) * Upgrade pip to 25.2 * Fix conflict in it_tracks * Update deprecated gh actions * Track changes for 8.19 * Fix test job which does not require the output of other jobs * Add backport reminder and reorganize the backporting actions. (#889) * Fix an error where ci arguments were not applied properly (#928) * Reduce filtering scope in CI workflow (#908) * Add pytest marker for cars * Address merge conflicts * Run serverless CI only on PRs targeting master (#859) * Accept pragma risks for joins esql * Make esql-full-text-functions a snapshot_only_challenge * Set index template for ingest_mode: data_stream (#849)
Introducing backport.py script that interacts with GH API to
Github Workflow changes:
Backport reminder workflow:
with the github.event_name.
fetches the list of PRs, merged the last X days from GH API
Inputs: