Remove hard dependency on OCP CRDs #1157
Merged
openshift-merge-bot[bot] merged 8 commits intoJun 1, 2026
Merged
Conversation
svghadi
reviewed
May 26, 2026
svghadi
reviewed
May 26, 2026
svghadi
reviewed
May 26, 2026
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
…able Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
assisted-by: claude-code for code,planning remove duplicate import, add checks before gitopsService and other OCP specific controller Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
anandrkskd
force-pushed
the
remove-OCP-hard-dependency
branch
from
f4b2f01 to
ae67f3e
Compare
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRuntime API discovery was expanded; cmd/main now logs discovery failures as errors and conditionally registers OpenShift-related schemes and controllers only when their APIs are detected. Controllers and cleanup logic skip OpenShift-specific behavior when APIs are absent; test helpers and TestMain allow overriding discovery flags in tests. ChangesConditional OpenShift API Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cmd/main.go`:
- Around line 135-137: The startup currently logs but continues when
util.InspectCluster() returns an error; change this to fail fast so the operator
doesn't run partially broken: after detecting err from util.InspectCluster(),
log the error with setupLog.Error(err, "unable to inspect cluster") and then
immediately terminate (e.g., call os.Exit(1) or log.Fatal/panic) instead of
continuing startup so discovery failures are explicit; update the error handling
in the main function where util.InspectCluster() is called.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5e966c53-53fe-4bda-828a-eabe196177c9
📒 Files selected for processing (5)
cmd/main.gocontrollers/gitopsservice_controller.gocontrollers/gitopsservice_controller_test.gocontrollers/util/test_util.gocontrollers/util/util.go
…, keep the check and register pattern same for all conditional api registration assisted-by: claude-code for code,planning Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
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 `@controllers/util/util.go`:
- Around line 92-105: InspectCluster currently returns early when
openShiftClusterFound is false which prevents verifyMonitoringAPI from running
and leaves monitoringAPIFound unset; remove the early return (or explicitly call
verifyMonitoringAPI before returning) so verifyMonitoringAPI always runs
regardless of openShiftClusterFound, ensuring monitoringAPIFound is discovered
and cmd/main.go can register the monitoring scheme/controller; update
InspectCluster (and references to openShiftClusterFound/monitoringAPIFound) so
only OpenShift-specific checks (eg. verifyRouteAPI, verifyConsoleAPI,
verifyTemplateAPI) are skipped when not OpenShift, while verifyMonitoringAPI
always executes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9ad0319d-98d7-42c1-8d27-a55343b68534
📒 Files selected for processing (6)
cmd/main.gocontrollers/argocd_controller.gocontrollers/gitopsservice_controller.gocontrollers/gitopsservice_controller_test.gocontrollers/util/test_util.gocontrollers/util/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/gitopsservice_controller_test.go
anandf
reviewed
May 28, 2026
anandf
reviewed
May 28, 2026
anandf
requested changes
May 28, 2026
Member
anandf
left a comment
anandf
left a comment
There was a problem hiding this comment.
posted few minor comments. Otherwise looks good to me.
remove non openshift check before config.openshift.io api check to make sure CRDs are not skipped, and not gate gitopsservice Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
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 `@controllers/util/util.go`:
- Around line 97-103: The early returns after calling verifyConfigAPI() and when
configAPIFound is false are discarding previously collected errors in errs;
update the control flow in the block around verifyConfigAPI(), configAPIFound
and errs so that you append verifyConfigAPI()'s error to errs and then return
the aggregated errs slice (or a combined error) instead of returning the raw
err, and similarly when configAPIFound is false return errs (or a combined
error) rather than nil; locate the logic using verifyConfigAPI(), the errs
variable and configAPIFound to implement returning the accumulated errors
consistently.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2bdef35b-f154-4bf9-bc96-6aede3fddc0a
📒 Files selected for processing (4)
cmd/main.gocontrollers/gitopsservice_controller_test.gocontrollers/util/test_util.gocontrollers/util/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/util/test_util.go
- controllers/gitopsservice_controller_test.go
Comment on lines
+97
to
+103
| if err := verifyConfigAPI(); err != nil { | ||
| errs = append(errs, err) | ||
| return err | ||
| } | ||
| if !configAPIFound { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Both early-return paths discard accumulated errors.
On Line 98 the error is appended to errs but Line 99 returns the raw err, dropping any OLM/Monitoring failures. Likewise Line 102 returns nil on non-OpenShift clusters, silently swallowing genuine OLM/Monitoring discovery errors collected at Line 90-95. Return the aggregated errs instead.
Suggested fix
if err := verifyConfigAPI(); err != nil {
errs = append(errs, err)
- return err
+ return stderrors.Join(errs...)
}
if !configAPIFound {
- return nil
+ return stderrors.Join(errs...)
}📝 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.
Suggested change
| if err := verifyConfigAPI(); err != nil { | |
| errs = append(errs, err) | |
| return err | |
| } | |
| if !configAPIFound { | |
| return nil | |
| } | |
| if err := verifyConfigAPI(); err != nil { | |
| errs = append(errs, err) | |
| return stderrors.Join(errs...) | |
| } | |
| if !configAPIFound { | |
| return stderrors.Join(errs...) | |
| } |
🤖 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 `@controllers/util/util.go` around lines 97 - 103, The early returns after
calling verifyConfigAPI() and when configAPIFound is false are discarding
previously collected errors in errs; update the control flow in the block around
verifyConfigAPI(), configAPIFound and errs so that you append
verifyConfigAPI()'s error to errs and then return the aggregated errs slice (or
a combined error) instead of returning the raw err, and similarly when
configAPIFound is false return errs (or a combined error) rather than nil;
locate the logic using verifyConfigAPI(), the errs variable and configAPIFound
to implement returning the accumulated errors consistently.
Signed-off-by: Anand Kumar Singh <anandrkskd@gmail.com>
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Member
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: svghadi 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 |
openshift-merge-bot
Bot
merged commit d4b8418
into
redhat-developer:master
17 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
assisted-by: claude-code
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Before this change, Openshift GitOps uses Routes and other Openshift specific tests as hard dependency. This PR removes this hard dependency that willl allow users to run gitops-operator on non OCP clusters. This PR also adds a check to see if promethus (monitoring.coreos.com) is present, if not it skips.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/GITOPS-9812
Test acceptance criteria:
How to test changes / Special notes to the reviewer: