WIP: UPSTREAM: <carry>: add AGENTS.md for AI code contributions #2503
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@kevinrizza: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kevinrizza The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kevinrizza: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/hold Should we consider inheriting this from upstream? |
openshift-ci
bot
added
the
do-not-merge/hold
It does seem like we shouldn't "step on" a change that's likely coming upstream but I'm conflicted on what the right answer should be. The file in the upstream PR details how to work with this repository in ways that we'd rather avoid here, and this file has proper context for this forked repository in ways that aren't applicable upstream. Should we keep both, with different names? Would agents even read both? Or should we step on the upstream file after all since it'd add potentially unnecessary complexity to the interpretation... |
|
My perspective here is that we probably want a carry patch that has our own separate AGENTS.md file. That pr upstream has been up for quite a while, but even if it does merge we probably want to have radically different behavior here vs upstream (hence my PR). I am obviously biased, but I'm happy to hear alternative suggestions. Another option would be that we do a merge of both if and when that change eventually lands upstream. |
|
It would be great to avoid conflicts as much as possible. WDYT @bertinatto? |
bertinatto
left a comment
bertinatto
left a comment
There was a problem hiding this comment.
I don’t feel strongly about this. If others think it adds value, I’m fine with merging it. My preference would be to improve the existing READMEs (agents do read them, right?) and address the gaps there, but I’m not opposed to this change.
|
|
||
| 1. **Remove all carry patches** from the current branch | ||
| 2. **Cherry-pick all new upstream commits** added between the two minor releases | ||
| 3. **Reapply all carry patches** on top of the new upstream base |
There was a problem hiding this comment.
We start with a clean branch from the upstream tag (e.g., v1.31.0) and cherry-pick all downstream carries from the current openshift/master branch. Then we merge our branch in openshift/master. The end result is openshift/master has the same code base as v1.31.0 plus the downstream carries.
| - `openshift-kube-apiserver/` - OpenShift API server customizations | ||
| - `openshift-kube-controller-manager/` - OpenShift controller manager customizations | ||
| - `README.openshift.md` - OpenShift fork documentation | ||
| - `REBASE.openshift.md` - Rebase process documentation |
There was a problem hiding this comment.
Perhaps just the OpenShift-specific directories/files?
| - `README.openshift.md` - OpenShift fork documentation | ||
| - `REBASE.openshift.md` - Rebase process documentation | ||
|
|
||
| ## Commit Message Conventions (CRITICAL) |
There was a problem hiding this comment.
This is already explained in the REBASE.openshift.md file; do we need to state this again?
| - **Related carries** should be squashed together to simplify future rebases | ||
|
|
||
| ### Enforcement | ||
| **Pull requests that do not follow these commit message conventions will be rejected by maintainers.** Every commit must use one of the `UPSTREAM:` prefixes listed above (except for direct upstream commits added during rebase). |
| If you identify a bug that affects Kubernetes generally (not just OpenShift): | ||
| 1. The fix should ideally go to upstream `kubernetes/kubernetes` first | ||
| 2. Then cherry-pick to OpenShift using `UPSTREAM: <PR number>:` format | ||
| 3. Only use `UPSTREAM: <carry>:` for truly OpenShift-specific behavior |
There was a problem hiding this comment.
These are good instrcutions, but I feel like they are not AI-specific. Maybe they would fit better in one of the README's we have (I'm OK with leaving them here as well).
| If changes are needed here, strongly consider: | ||
| 1. Can this be fixed upstream first? | ||
| 2. Is there a way to achieve this with less invasive changes? | ||
| 3. Can this be done via a plugin or extension point? |
There was a problem hiding this comment.
I wonder if this will discourage agents from fixing code in k8s if I have my openshift/master checked out.
| 2. **Understand the context**: Is this addressing an upstream or downstream concern? | ||
| 3. **Minimize divergence**: Prefer smaller, more targeted changes | ||
| 4. **Use correct prefixes**: Every commit must have an `UPSTREAM:` prefix | ||
| 5. **Check for upstream fixes**: Before implementing a fix, search if it exists upstream |
There was a problem hiding this comment.
This one is very useful. I wonder if the agent will need instructions on how to go through the upstream issues/PRs in GitHub.
| 3. **Minimize divergence**: Prefer smaller, more targeted changes | ||
| 4. **Use correct prefixes**: Every commit must have an `UPSTREAM:` prefix | ||
| 5. **Check for upstream fixes**: Before implementing a fix, search if it exists upstream | ||
| 6. **Suggest testing**: Always recommend running `make test` and `make verify` |
There was a problem hiding this comment.
This takes a lot of time, maybe we should suggest running individual tests.
|
|
||
| 1. **Mixing generated and code changes** in the same commit | ||
| 2. **Using wrong UPSTREAM prefix** (causes rebase issues) | ||
| 3. **Modifying core Kubernetes code** without understanding if it should be upstream |
There was a problem hiding this comment.
Maybe this should be something like: add downstream-only commits (prefixed with UPSTREAM msg) that should be upstream
5 participants
Adding AGENTS.md file for code generation tools to interact with this repository. Mostly focuses on the kinds of changes that should be made to this repo.