-
Notifications
You must be signed in to change notification settings - Fork 43
Add ocp-style-rebase command #43
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
Conversation
The command performs a rebase of an OpenShift fork of an upstream repo to the latest upstream release.
|
I'm not entirely sure this is a good idea for a task a Claude slash command should be taking on. Especially given agents can ignore instructions, for something as critical as a rebase we don't want to introduce agentic uncertainty :( Have we considered implications if this fails silently / hallucinates? We also have rebasebot already, that does something very similar? https://github.com/openshift-eng/rebasebot |
|
Yeah, so the main concern is - for larger rebases where it is hard to verify commits individually without doing full rebase yourself, it will be hard to ascertain correctness of a rebase performed via LLM. |
|
In the storage repos, we usually have just a single What I found useful was:
What is not so great:
|
|
/lgtm |
|
@mpatlasov: changing LGTM is restricted to collaborators In response to this:
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. |
plugins/openshift/commands/rebase.md
Outdated
| Make sure they are links to changelogs, not tags. | ||
| Print list of the links. | ||
|
|
||
| 14. Create a github pull request against the OpenShift github repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you need to specify a "remote" to create the PR from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, claude is smart enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a write operation, I think it would be a sensible guardrail to be explicit about always pushing against the user remote and never pushing against the openshift/upstream remote. I've seen claude pushing against my openshift remote instead of my fork when using commands that create PRs.
Also how should claude prefer to create the PR? via GH cli? mcp? should this be part of ### Pre-requisites?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a light preference for GH Cli, MCP exhausts context quickly (as does each tool invocation) and these commands are predictable :)
|
|
||
| 7. Find the merge base of the `openshift/master` and `$previous_tag` by running `git merge-base openshift/master $previous_tag`. We will use this merge base as `$mergebase`. | ||
|
|
||
| 8. Prepare `commits.tsv` tab-separated values file containing the set of carry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should steps 1..8 be a bash scripted skill to favor determinism and reduce changes of claude derailing? cc @theobarberbany
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I would take that approach here. I think hooks (if appropriate) and scripting deterministic steps instead of prompt glue is generally a better approach!
UserPromptSubmit should be able to hook in at the start of the invocation of the command, hopefully making this more predictable :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a claude code native way to scope hooks to specific commands within a plugin though. Let's leave this aside this PR and evaluate if we want to introduce a handcrafted pattern for that separately. Let's ship it, learn by use it and iterate
|
/label tide/merge-method-squash |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, jsafrane, mpatlasov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The command performs a rebase of an OpenShift fork of an upstream repo to the latest upstream release.
Example of output: openshift/aws-ebs-csi-driver#294. I heavily edited the PR description, claude does not follow the instructions well :-(. But when it comes to cherry picking and squashing commits and resolving conflicts, it's pretty solid.
New example: openshift/csi-external-snapshotter#189
No edits in the PR description.