openshift-mcp-server: Auto-ff daily - #83135
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
3696cd3 to
7c615b6
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a CI fast-forward command, registers its step and workflow, and schedules a daily job to update the ChangesMCP server fast-forward automation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduledJob
participant CIWorkflow
participant FastforwardStep
participant GitHubRepository
ScheduledJob->>CIWorkflow: run daily at 06:00 UTC
CIWorkflow->>FastforwardStep: invoke fast-forward workflow
FastforwardStep->>GitHubRepository: clone destination branch
FastforwardStep->>GitHubRepository: pull source branch with --ff-only
FastforwardStep->>GitHubRepository: push release-0.5
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
7c615b6 to
050c2ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-commands.sh`:
- Around line 30-36: Update the destination-branch clone handling in the
fast-forward command script so only a confirmed missing $DESTINATION_BRANCH
enters the fallback clone and branch-creation path. Preserve and propagate
authentication, network, repository, and server errors instead of treating them
as absent branches; use the existing git/logging flow and symbols without
changing successful clone behavior.
- Around line 16-25: Update the fast-forward script’s startup validation to
check both SOURCE_BRANCH and DESTINATION_BRANCH before the settings log, using
${SOURCE_BRANCH:-} so an unset or empty source is rejected safely under set -u.
Preserve the existing error-and-exit behavior, then allow logging and subsequent
git clone operations only after both branch values are valid.
In
`@ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-workflow.yaml`:
- Around line 3-5: Update the workflow’s steps declaration to include empty pre
and post phases alongside the existing test phase, preserving the
openshift-mcp-server-fastforward test reference. Then run make
validate-step-registry to verify the step registry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b753127d-7919-4dc0-9a94-015ed402f3e1
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/openshift-mcp-server/openshift-openshift-mcp-server-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (7)
ci-operator/config/openshift/openshift-mcp-server/openshift-openshift-mcp-server-main.yamlci-operator/step-registry/openshift/mcp-server/fastforward/OWNERSci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-commands.shci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-ref.metadata.jsonci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-ref.yamlci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-workflow.metadata.jsonci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-workflow.yaml
050c2ad to
f3e560e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-commands.sh`:
- Line 5: Update the HOME initialization around the mktemp command so its
failure status is preserved: first assign the mktemp result to a temporary
variable, then export HOME from that variable. Keep the existing mktemp template
and ensure failures stop before any subsequent directory change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2c8b070e-d8b4-4cf1-baab-5e01e9d0a46b
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/openshift-mcp-server/openshift-openshift-mcp-server-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (7)
ci-operator/config/openshift/openshift-mcp-server/openshift-openshift-mcp-server-main.yamlci-operator/step-registry/openshift/mcp-server/fastforward/OWNERSci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-commands.shci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-ref.metadata.jsonci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-ref.yamlci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-workflow.metadata.jsonci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-workflow.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- ci-operator/step-registry/openshift/mcp-server/fastforward/OWNERS
- ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-ref.metadata.json
- ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-workflow.yaml
- ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-ref.yaml
|
|
||
| set -euo pipefail | ||
|
|
||
| export HOME="$(mktemp -d -t ff-XXXXX)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Preserve the mktemp failure status.
export HOME="$(mktemp -d -t ff-XXXXX)" returns the status of export, not the command substitution. If mktemp fails, set -e can miss the original failure and report a later cd error. Assign the result before exporting HOME.
Proposed fix
-export HOME="$(mktemp -d -t ff-XXXXX)"
+home_dir="$(mktemp -d -t ff-XXXXX)"
+export HOME="$home_dir"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export HOME="$(mktemp -d -t ff-XXXXX)" | |
| home_dir="$(mktemp -d -t ff-XXXXX)" | |
| export HOME="$home_dir" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/openshift/mcp-server/fastforward/openshift-mcp-server-fastforward-commands.sh`
at line 5, Update the HOME initialization around the mktemp command so its
failure status is preserved: first assign the mktemp result to a temporary
variable, then export HOME from that variable. Keep the existing mktemp template
and ensure failures stop before any subsequent directory change.
Source: Linters/SAST tools
f3e560e to
12a4597
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test config /pj-rehearse periodic-ci-openshift-openshift-mcp-server-main-fast-forward-latest-release |
|
@2uasimojo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
12a4597 to
1b34c2c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/pj-rehearse periodic-ci-openshift-openshift-mcp-server-main-fast-forward-latest-release |
|
@2uasimojo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1b34c2c to
05824d6
Compare
|
/pj-rehearse periodic-ci-openshift-openshift-mcp-server-main-fast-forward-latest-release |
|
@2uasimojo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/pj-rehearse skip Failed to load the cred. I think this is the expected result for rehearsals. And I think we're just supposed to merge and iterate now. [Later] Confirmed the above. |
|
@2uasimojo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
cajieh
left a comment
cajieh
left a comment
There was a problem hiding this comment.
Rehearse failed with secrets "github-credentials-openshift-merge-robot" not found. It like that secret is only on app.ci/core-ci and this job is on build01, should we pin the job to app.ci/core-ci or bootstrap the secret onto the build farm?
Added a few more comments.
| @@ -0,0 +1,77 @@ | |||
| periodics: | |||
| - agent: kubernetes | |||
| cluster: build01 | |||
There was a problem hiding this comment.
Given github-credentials-openshift-merge-robot isn’t on the build farm, should this job be pinned to app.ci or core-ci instead of build01?
There was a problem hiding this comment.
This file is generated.
IIUC the secret not existing for rehearsals is deliberate (otherwise a malicious or merely hapless actor would be able to do privileged things just by proposing and rehearsing a PR). It should be present when this runs for real.
There was a problem hiding this comment.
This ended up being a good callout. I'm trying to figure out how to pin the job to a cluster. (I don't think I'm allowed to change it here, because there are CI jobs that make sure that generated files aren't changed. But I've been surprised on that count in the past.)
| log " SOURCE_BRANCH = $SOURCE_BRANCH" | ||
| log " DESTINATION_BRANCH = $DESTINATION_BRANCH" | ||
|
|
||
| repo_url="https://github.com/${REPO_OWNER}/${REPO_NAME}.git" |
There was a problem hiding this comment.
After the secret works, won’t /pj-rehearse run a real git push to release-0.5? Should we no-op (skip the push) during rehearsals so we don’t update that branch just to test the PR?
There was a problem hiding this comment.
As noted, the secret won't (and shouldn't) work in rehearsals. And the failure occurs before this script is invoked, so rehearsal-only logic wouldn't help.
| @@ -0,0 +1,28 @@ | |||
| ref: | |||
| as: openshift-mcp-server-fastforward | |||
| from_image: | |||
There was a problem hiding this comment.
This needs git and the image is ocp/5.0:cli, right?
Does the image include git?
There was a problem hiding this comment.
Gah, no, it doesn't. You won't believe this, but I've been lied to by an LLM.
Will fix. (Though still, we won't be able to validate until merged.)
| log " REPO_NAME = $REPO_NAME" | ||
| log " SOURCE_BRANCH = $SOURCE_BRANCH" | ||
| log " DESTINATION_BRANCH = $DESTINATION_BRANCH" | ||
|
|
There was a problem hiding this comment.
Since DESTINATION_BRANCH can be "", and set -u won’t catch that, should we add an early non-empty check so we fail with a clear error before git clone?
There was a problem hiding this comment.
Talked through this with the rabbit above. Since we control the invocation very tightly, I'm pretty unworried about this kind of thing. The error we would get organically isn't inscrutable enough to warrant the extra script clutter IMO:
$ git clone -b "" https://github.com/openshift/hive.git
Cloning into 'hive'...
fatal: Remote branch not found in upstream origin
But if you feel strongly about it I'll add the extra checks.
There was a problem hiding this comment.
It would be nice if the default field wasn't required... but I've thought of a compromise: what if I made it default: unset so the organic error message would be like fatal: Remote branch unset not found in upstream origin?
There was a problem hiding this comment.
Yes, the default: unset is fine.
| - namespace: ci | ||
| name: github-credentials-openshift-merge-robot | ||
| mount_path: /etc/github | ||
| env: |
There was a problem hiding this comment.
nit: Should REPO_OWNER and REPO_NAME be documented here as ci-operator-injected env vars for clarity?
There was a problem hiding this comment.
I looked around and that's not being done anywhere else. (Also this file has magical semantics. I don't know this for sure, but it may error if I try to declare vars that are "owned" elsewhere.)
I wouldn't actually object to hardcoding openshift/openshift-mcp-server -- as currently written this workflow could technically be reused by others, but the name of the workflow would look weird if they did. But for now I'll leave it.
Co-Authored-By: claude
05824d6 to
d2c17e4
Compare
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
removed
the
rehearsals-ack
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse skip Configs are still okay. Script won't run in rehearsal due to (intentional) unavailability of creds secret. |
|
@2uasimojo: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
@cajieh Updates are bundled with a rebase, apologies. The two changes are here: |
|
Looks good, but I'd defer to @Cali0707 or @grokspawn for a second pair of eyes. |
| documentation: |- | ||
| The branch to fast-forward FROM. | ||
| - name: DESTINATION_BRANCH | ||
| default: "unset" |
There was a problem hiding this comment.
I think there's a danger that someone oopses and doesn't set DESTINATION_BRANCH, so it's then set to "unset" (but it's never checked and someone would have to figure out that it means the string-literal "unset" rather than the logical concept.
It might be better to default to empty-string and verify against that, with a clear error.
I'll leave that as a nit since I'm not sure how real the danger is.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, grokspawn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@2uasimojo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Replaces openshift#83135/openshift#83296, putting the job in infra-periodics per protocol. Assisted-By: cursor/grok-4.5
Replaces openshift#83135/openshift#83296, putting the job in infra-periodics per protocol. Assisted-By: cursor/grok-4.5
Co-Authored-By: claude
3 participants
Co-Authored-By: claude
Summary by CodeRabbit
This PR adds daily OpenShift CI automation for the
openshift/openshift-mcp-serverrepository.release-0.5branch through theopenshift-mcp-server-fastforwardworkflow.--ff-onlybefore pushing updates.