Skip to content

Conversation

@rashmigottipati
Copy link
Member

@rashmigottipati rashmigottipati commented Dec 11, 2025

Fixes: OPRUN-4268

Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>

---

## Understanding the Rebase Process
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth including that these processes are crystallized in openshift/operator-framework-tooling?

- **Updated** if upstream code has changed in conflicting ways
- **Maintained** by the OpenShift team indefinitely

**Carry patches should only be added if a change absolutely cannot be accepted upstream.** Every carry patch represents ongoing manual work for the maintainers through every upstream release.
Copy link
Contributor

Choose a reason for hiding this comment

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


**When to use:** ONLY during the rebase process itself

**AI tools should NEVER suggest these** in regular pull requests - these are added by maintainers when rebasing to a new upstream version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025
1. **Understand the intent:** Is this fixing an OpenShift-specific issue or an upstream bug?

2. **Choose the right approach:**
- If it's an upstream bug → Should be fixed in `operator-framework/operator-controller` first, then cherry-picked
Copy link
Contributor

Choose a reason for hiding this comment

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

Not cherry-picked. The downstream-related tooling should be allowed to execute, replaying commits against our fork of the upstream branch, once that has been fast-forwarded.


## 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).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is that any PR not following these conventions will fail the CI check by commitchecker (though we've definitely encountered some bugs where it did not!!!).

2. **Choose the right approach:**
- If it's an upstream bug → Should be fixed in `operator-framework/operator-controller` first, then cherry-picked
- If it's OpenShift-specific → Use `UPSTREAM: <carry>:` prefix
- If it's generated code → Will be handled by `make manifests` with `UPSTREAM: <drop>:`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to say that agents should resolve via an UPSTREAM: <drop> PR if and only if (IFF) those changes are needed to resolve current failures, and local re-generation of the content cannot (for example, if multiple PRs influence generated content in ways which cannot be trivially merged).

If you identify a bug that affects operator-controller generally (not just OpenShift):

1. **The fix should ideally go to upstream `operator-framework/operator-controller` first**
2. **Then cherry-pick** to OpenShift using `UPSTREAM: <PR number>:` format
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want agents performing this step. Instead it's probably better to say that they should wait for the upstream change to be downstreamed using the $tooling on its scheduled interval.
Otherwise, we're going to end up with a bunch of PR-labeled changes duplicating the work of the bumper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, we avoid using UPSTREAM: <pr number> because we sync everything from upstream. It would have to be an exceptional case to use UPSTREAM: <pr number>, and in fact, it might be preferred to use UPSTREAM: <drop> because the fix will eventually come down from upstream.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, updated in the latest commit


1. **The fix should ideally go to upstream `operator-framework/operator-controller` first**
2. **Then cherry-pick** to OpenShift using `UPSTREAM: <PR number>:` format
3. **Only use `UPSTREAM: <carry>:`** for truly OpenShift-specific behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
3. **Only use `UPSTREAM: <carry>:`** for truly OpenShift-specific behavior
3. **Only use `UPSTREAM: <carry>:`** for truly OpenShift-specific behavior for content which is not generated

| OpenShift test (OTE) | `UPSTREAM: <carry>:` | ❌ NO - Downstream only |
| OpenShift integration | `UPSTREAM: <carry>:` | ❌ NO - Downstream only |
| OpenShift config | `UPSTREAM: <carry>:` | ❌ NO - Downstream only |
| Remove upstream file | `UPSTREAM: <drop>:` | ❌ NO - Downstream only |
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think if a case where we would want to follow this path.

Copy link
Contributor

Choose a reason for hiding this comment

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

An UPSTREAM: <drop> is generally used as a temporary fix, since it's gone during the next sync.
If we did want to remove an upstream file (e.g. .github directory), it would still be a <carry> commit. However, we try to actually avoid that. The bumper actually deletes some of these files itself with a <drop> because it can handle the process better than trying to carry over a <carry> commit and the conflicts that can create.

| OpenShift integration | `UPSTREAM: <carry>:` | ❌ NO - Downstream only |
| OpenShift config | `UPSTREAM: <carry>:` | ❌ NO - Downstream only |
| Remove upstream file | `UPSTREAM: <drop>:` | ❌ NO - Downstream only |
| Generated files | `UPSTREAM: <drop>:` | ❌ NO - Regenerate with `make manifests` |
Copy link
Contributor

Choose a reason for hiding this comment

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

This contradicts previous guidance here which implies that generated stuff which we need to temporarily resolve until some later PR should use this process.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it is something we do, and something I have done.

@grokspawn
Copy link
Contributor

There's some duplicated and internally contradictory content here. Could you re-run this through claude with a "be concise and consistent" prompt modification? If you also direct it to resolve PR comments then you should be able to get to a happy update in a single step.

Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025
Comment on lines 188 to 189
| General bug fix | `UPSTREAM: <PR>:` | ✅ YES - Fix upstream, then cherry-pick |
| General feature | `UPSTREAM: <PR>:` | ✅ YES - Add upstream, then cherry-pick |
Copy link
Contributor

Choose a reason for hiding this comment

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

This ought to be "Fix upstream, then let the tooling synchronize"

@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

TL;DR: we don't cherry-pick to the main branch. We allow the bumper (in openshift/operator-framework-tooling) synchronize from upstream.

However, cherry-picking would be done on release branches, but since most downstream work is on the main branch, cherry-picking is uncommon.

We would almost never see UPSTREAM: pr-number commits. because they would create a conflict on the next upstream sync. Instead, we might do an UPSTREAM: <drop> commit to pull down a fix early, as it then gets dropped on the next sync.

UPSTREAM: <carry> is used for most changes, and 99% of the time, those changes ought to be limited to the openshift directory. Code outside of the openshift directory should generally not be modified downstream; it should be modified upstream and then synced downstream.

UPSTREAM: <drop> is for temporary commits (e.g. the upstream PR example) that are to be dropped on the next sync. They are used by the bumper to create commits that are easier to recreate than to rebase/cherry-pick. For example: UPSTREAM: <drop>: remove upstream GitHub configuration

EDIT: You updated the file as I was writing this... so looking 👀 again.

**Upstream Repository:** https://github.com/operator-framework/operator-controller

**Key Constraints for AI Agents:**
- This fork is **rebased regularly** against upstream (with each upstream release)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, it's done daily, not on upstream releases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this in the latest commit

Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025
@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

This version is much better. I think I had just one comment on the frequency of the synchronization.

Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025
@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025
3. **Check upstream first** before fixing bugs - should it go upstream?
4. **Separate commits:** Never mix code changes with generated files
5. **Run verification:** `make verify` and `make test-unit` before completion
6. **Sign commits:** Use `git commit -s` for DCO
Copy link
Member

Choose a reason for hiding this comment

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

There's a difference between signing a commit, and including DCO. -s is DCO. -S is sign cryptographically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we have root-level DCO file to cover this. So while signing is desired, it's not a required step for DCO. And it isn't cryptographic identity attestation.

Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2025
@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

/lgtm

@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

/approve

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 11, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankitathomas, grokspawn, rashmigottipati, tmshort

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Dec 11, 2025
@tmshort
Copy link
Contributor

tmshort commented Dec 11, 2025

/retitle NO-ISSUE: Add AGENTS.md to allow agents to make contributions to the codebase

@openshift-ci openshift-ci bot changed the title Add AGENTS.md to allow agents to make contributions to the codebase NO-ISSUE: Add AGENTS.md to allow agents to make contributions to the codebase Dec 11, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 11, 2025
@openshift-ci-robot
Copy link

@rashmigottipati: This pull request explicitly references no jira issue.

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 openshift-eng/jira-lifecycle-plugin repository.

@rashmigottipati rashmigottipati changed the title NO-ISSUE: Add AGENTS.md to allow agents to make contributions to the codebase OPRUN-4268: Add AGENTS.md to allow agents to make contributions to the codebase Dec 11, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 11, 2025

@rashmigottipati: This pull request references OPRUN-4268 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set.

In response to this:

Fixes: OPRUN-4268

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 openshift-eng/jira-lifecycle-plugin repository.

@rashmigottipati
Copy link
Member Author

/refresh

@bandrade
Copy link

/label qe-approved
/verified by @bandrade

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 11, 2025
@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Dec 11, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 11, 2025

@rashmigottipati: This pull request references OPRUN-4268 which is a valid jira issue.

In response to this:

Fixes: OPRUN-4268

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

@bandrade: This PR has been marked as verified by @bandrade.

In response to this:

/label qe-approved
/verified by @bandrade

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 11, 2025

@rashmigottipati: all tests passed!

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 47053e9 into openshift:main Dec 11, 2025
11 checks passed
@rashmigottipati rashmigottipati deleted the add-agents-file branch December 11, 2025 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants