Add --always-run-hooks flag to run lifecycle hooks even when no rebase is needed by mpryc · Pull Request #67 · openshift-eng/rebasebot

mpryc · 2025-08-23T09:17:18Z

Summary

Maintains full backward compatibility.
Add new --always-run-hooks CLI flag to enable hook execution even when no rebase is required
Update hook execution logic to run main lifecycle hooks (pre-rebase, pre-carry-commit, post-rebase) when flag is enabled
Enhance PR handling logic to support hook-driven updates without requiring upstream changes
Add comprehensive test coverage for the new functionality
Update documentation with usage examples and behavior explanation

Key Changes

CLI & Core Logic

New flag: --always-run-hooks opt-in flag in cli.py
Hook execution: Modified bot.py to run main lifecycle hooks even without rebase when flag is enabled
PR logic: Enhanced PR creation/update logic to handle hook-driven changes. Only open PRs are taken into consideration. This is to ensure previously merged rebases with closed PRs won't get updated when no upstream changes are discovered.
Dry run support: Proper dry-run handling for the new execution paths. Dry-run doesn't just stop without checking for existing PR or without comparing git diff, it does those steps, however it does not execute commits or PR creations.

Testing

Added unit tests for CLI argument parsing and flag propagation
Added integration tests for hook execution scenarios
Tests cover both enabled and disabled flag behavior
Manual tests were performed on the migtools/kopia repository.

Documentation

Updated README.md with detailed explanation of the new feature
Added usage examples including Go module updates scenario
Documented hook execution behavior and limitations

Use Cases

This feature enables scenarios like:

Automated dependency updates (e.g., Go modules) without waiting for upstream changes
Regular maintenance tasks and code generation
Keeping downstream repositories synchronized with tooling updates

Behavior

When --always-run-hooks is enabled:

Main lifecycle hooks run even if no rebase is needed
Action-specific hooks (push, PR creation) still only run when those actions occur
Built-in hooks (like --update-go-modules) execute as usual
Existing rebase logic and execution order remain unchanged

Co-Authored-By: Claude noreply@anthropic.com

openshift-ci · 2025-08-23T09:17:29Z

Hi @mpryc. Thanks for your PR.

I'm waiting for a openshift-eng member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mpryc · 2025-08-23T09:57:08Z

This change is required to achieve the full rebase process for all OADP modules described in:
https://github.com/oadp-rebasebot/oadp-rebase/blob/oadp-dev/README.md

Some modules need dependency updates to reference freshly rebased upstream versions, even when the modules themselves have no upstream changes requiring a rebase.

mpryc · 2025-08-24T19:57:46Z

/hold - need to add one more condition where destination branch already contains the change which is being made by hook (no rebase situation).

damdo · 2025-08-29T14:02:11Z

/assign @RadekManak

mpryc · 2025-09-01T07:46:26Z

/unhold

RadekManak · 2025-09-02T10:04:22Z

/ok-to-test

RadekManak

I guess the feature makes sense. Please take more time reviewing llm generated code before submitting.

RadekManak · 2025-09-02T10:47:21Z

 ```

-Example 3. Rebasebot usage in OpenShift CI pipeline.
+Example 3. Run Go module updates even when no rebase is needed.


Let's keep the examples minimal the main readme. I don't think the feature needs example.

RadekManak · 2025-09-03T14:16:25Z

+        args.post_rebase_hook = [hook_ref]
+        args.pre_carry_commit_hook = None
+        args.pre_push_rebase_branch_hook = None
+        args.pre_create_pr_hook = None


Each hook should create different sentinel file to test all were executed.

RadekManak · 2025-09-03T14:41:16Z

        assert passed_gh_app_provider._cloner_app_credentials.app_id == 137497  # default value
        assert passed_gh_app_provider._app_credentials.app_key == b'some cool content'
        assert passed_gh_app_provider._cloner_app_credentials.app_key == b'some cool content'
+


I don't see the value of these tests. They are way too confusing for the little benefit of ensuring that the argument parsing works. We use an external library for parsing anyway. I think we should remove these.

RadekManak · 2025-09-10T12:49:20Z

                # Case 4: we created a PR, but no changes were done to the repos after that.
                # Just infrom that the PR is in a good shape.
                message = f"PR {pr_url} already contains the latest changes"
+        elif pr_url is not None and pr_url != "":


I don't think this function needs always_run_hooks as parameter.

RadekManak · 2025-09-10T12:53:46Z

            f"{ex}",
        )
        return False



I don't think there is a need to run. Pre push and pre PR hooks if we are in dry_run. I would expected these to interact with the network since they also depend on external factors like PR being open or branch pushed.

Keeping the dry_run cutoff in one place makes the code cleaner.

@mpryc Everything below this is behind if(dry_run): This is in unnecessarily complex.
Revert:

if dry_run: logging.info("Dry run mode is enabled. Do not create a PR.") return True push_required = _is_push_required(gitwd, rebase) if needs_rebase else False

And then you can clean up the dry run checks below.

RadekManak · 2025-09-10T12:55:21Z

+
+Enable this behavior with the opt-in `--always-run-hooks` flag:
+
+```txt


This does not need codeblock exampe example since it is just a bool param.

…e is needed This enables users to run maintenance tasks, dependency updates, and code generation hooks regardless of whether upstream changes require a rebase. Signed-off-by: Michal Pryc <mpryc@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>

1. Cleaned up README.md to remove unnecessary examples. 2. Removed always_run from the _report_result 3. Dry run does not trigger Pre push and Pre PR hooks 4. Removed unnecessary tests for new flag 5. Modified tests to include different sentinel files 6. Updated tests to cover above fixes Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Michal Pryc <mpryc@redhat.com>

RadekManak · 2025-10-08T10:48:03Z

            f"{ex}",
        )
        return False



@mpryc Everything below this is behind if(dry_run): This is in unnecessarily complex.
Revert:

if dry_run: logging.info("Dry run mode is enabled. Do not create a PR.") return True push_required = _is_push_required(gitwd, rebase) if needs_rebase else False

And then you can clean up the dry run checks below.

RadekManak · 2025-10-08T11:20:21Z

@@ -635,14 +658,13 @@ def _report_result(needs_rebase: bool, pr_available: bool, pr_url: str, dest_url
            # We updated the exiting PR.
            message = f"I updated existing rebase PR: {pr_url}"
    else:


This is missing a case when updating existing PR, but only hooks made updates.

@RadekManak I have made changes and tested - works great on the project where no changes were during rebase, but only hooks made changes, however there is one part which I hope won't break others:

The check if push is required is now happening without if needs_rebase, so always it's being executed to see if actual push is requied.

https://github.com/openshift-eng/rebasebot/pull/67/files#diff-ef0ae8c4429f5305666f568bf98e90a9b457af4826d674115cbb6c8e60a802f8R828

Similarly for:
https://github.com/openshift-eng/rebasebot/pull/67/files#diff-ef0ae8c4429f5305666f568bf98e90a9b457af4826d674115cbb6c8e60a802f8R871

- Always check actual git diffs instead of needs_rebase flag to determine if push/PR is required, fixing bug where hook-generated changes weren't pushed when --always-run-hooks is enabled and no rebase was needed - Simplify dry_run handling by checking early and returning, removing nested dry_run conditionals throughout the code - Add missing case in _report_result for when hooks update an existing PR (Case 4: hooks made changes to existing PR) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Michal Pryc <mpryc@redhat.com>

RadekManak · 2025-10-13T14:11:34Z

/approve
/lgtm

openshift-ci · 2025-10-13T14:11:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpryc, RadekManak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [RadekManak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci Bot requested review from JoelSpeed and damdo August 23, 2025 09:17

openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2025

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2025

openshift-ci Bot assigned RadekManak Aug 29, 2025

mpryc force-pushed the post-scripts-no-rebase branch 2 times, most recently from 4d2916d to e53ef05 Compare August 31, 2025 19:01

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2025

openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 2, 2025

RadekManak suggested changes Sep 10, 2025

View reviewed changes

mpryc mentioned this pull request Sep 18, 2025

Add tide for the oadp-1.4 branch for oadp-non-admin openshift/release#69404

Merged

mpryc and others added 2 commits September 29, 2025 18:30

mpryc force-pushed the post-scripts-no-rebase branch from e53ef05 to c431f4a Compare October 8, 2025 10:22

RadekManak suggested changes Oct 8, 2025

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2025

openshift-merge-bot Bot merged commit b8d8bc5 into openshift-eng:main Oct 13, 2025
4 checks passed


		Enable this behavior with the opt-in `--always-run-hooks` flag:

		```txt

Conversation

mpryc commented Aug 23, 2025

Summary

Key Changes

CLI & Core Logic

Testing

Documentation

Use Cases

Behavior

Uh oh!

openshift-ci Bot commented Aug 23, 2025

Uh oh!

mpryc commented Aug 23, 2025

Uh oh!

mpryc commented Aug 24, 2025

Uh oh!

damdo commented Aug 29, 2025

Uh oh!

mpryc commented Sep 1, 2025

Uh oh!

RadekManak commented Sep 2, 2025

Uh oh!

RadekManak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RadekManak commented Oct 13, 2025

Uh oh!

openshift-ci Bot commented Oct 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants