Skip to content

ROSAENG-67544: Remove ocm-cli dependency from ocm-common - #169

Merged
andreadecorte merged 1 commit into
openshift-online:mainfrom
redhat-chai-bot:ROSAENG-67544-remove-ocm-cli-dep
Sep 23, 2026
Merged

andreadecorte merged 1 commit into
openshift-online:mainfrom
redhat-chai-bot:ROSAENG-67544-remove-ocm-cli-dep

Conversation

@redhat-chai-bot

@redhat-chai-bot redhat-chai-bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Remove the circular module dependency on github.com/openshift-online/ocm-cli from ocm-common. A shared library should not depend on its CLI consumer.

This dependency was introduced when the config and connection-builder packages were migrated from ocm-cli (commit 97a7ba6).

Changes

pkg/ocm/config/config.go

  • Replaced import of github.com/openshift-online/ocm-cli/pkg/properties with github.com/openshift-online/ocm-common/pkg/ocm/consts
  • Changed properties.KeyringEnvKey → consts.KeyringEnvKey (the constant already existed locally with the same value "OCM_KEYRING")

pkg/ocm/connection-builder/connection.go

  • Removed import of github.com/openshift-online/ocm-cli/pkg/debug
  • Simplified getLogger() to always use glog.Level(1) as the default log level
  • Callers who need debug logging can use the existing WithLogger() API to provide their own logger

pkg/ocm/connection-builder/connection_test.go (new)

  • Added Ginkgo tests verifying default logger behavior and WithLogger() override

go.mod / go.sum

  • Ran go mod tidy to remove github.com/openshift-online/ocm-cli from dependencies

Validation

  • grep -r "ocm-cli" --include="*.go" . — no matches
  • grep "ocm-cli" go.mod — not present
  • make test — all 26 test packages pass

Jira

ROSAENG-67544


AI-generated. Review for accuracy.

@andreadecorte requested in Slack thread

Summary by CodeRabbit

  • Changes
    • The default logger now uses standard verbosity rather than automatically enabling debug verbosity. To use debug logging, provide a logger explicitly.
  • Tests
    • Added coverage for default logger creation and for using a caller-provided logger.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 45537315-5aab-4798-8a13-bc4dc877d700

📥 Commits

Reviewing files that changed from the base of the PR and between ab50579 and 5f310e5.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (4)
  • go.mod
  • pkg/ocm/config/config.go
  • pkg/ocm/connection-builder/connection.go
  • pkg/ocm/connection-builder/connection_test.go
💤 Files with no reviewable changes (1)
  • go.mod

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The changes remove two module dependencies, switch the keyring environment-variable constant source, and make the connection builder use standard verbosity for its default logger. New tests cover default and caller-supplied loggers.

Changes

OCM dependency and logger updates

Layer / File(s) Summary
Update keyring constant and module dependencies
pkg/ocm/config/config.go, go.mod
IsKeyringManaged now uses consts.KeyringEnvKey. go.mod removes the ocm-cli and pflag dependencies.
Set and test default logger behavior
pkg/ocm/connection-builder/connection.go, pkg/ocm/connection-builder/connection_test.go
getLogger always creates the default logger at glog.Level(1). Tests cover default logger creation and use of a logger supplied through WithLogger.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 5f310

The dependency cleanup and keyring substitution show no concrete integration failure. Callers needing debug verbosity can provide a logger; no material merge-blocking risk is established.

🚥 Pre-merge checks | ✅ 9 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Ai-Attribution ⚠️ Warning AI use is mentioned in the pull-request description, which labels the content “AI-generated.” The reviewed commit includes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>, but it does not in… Remove the AI tool’s Co-Authored-By trailer. Add the applicable Red Hat attribution trailer, such as Assisted-by: Claude Opus 4.6 or Generated-by: Claude Opus 4.6, to the commit.
✅ Passed checks (9 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: removing the ocm-cli dependency from ocm-common.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto ✅ Passed PASS: The pull request changes dependency imports, logger defaults, tests, and module metadata only. The changed lines introduce no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, custom cryptography, or se…
Container-Privileges ✅ Passed The pull request changes only Go source, tests, and Go module files. It changes no YAML, JSON, or TOML manifests, and its added lines contain none of the listed privilege settings. No explicit contain…
No-Sensitive-Data-In-Logs ✅ Passed The pull request adds no logging calls or sensitive-value formatting. The only logging behavior change sets the default logger to glog.Level(1); the previous code could use the more verbose glog.Level…
No-Hardcoded-Secrets ✅ Passed The PR adds no hardcoded secrets. Scanning its 57 added lines found no secret indicators, credential assignments, URLs with embedded credentials, or long base64-like strings. The changed config code u…
No-Injection-Vectors ✅ Passed The pull request does not introduce any listed injection vector. The changed lines only replace a constant import, read an environment variable with os.Getenv, adjust logger configuration, update modu…
Full details: Ai-Attribution

Explanation

AI use is mentioned in the pull-request description, which labels the content “AI-generated.” The reviewed commit includes Co-Authored-By: Claude Opus 4.6 &lt;noreply@anthropic.com&gt;, but it does not include an Assisted-by or Generated-by trailer. The AI co-author trailer also violates the instruction to flag Co-Authored-By attribution for AI tools.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

- Replaced ocm-cli/pkg/properties.KeyringEnvKey with local consts.KeyringEnvKey
- Removed ocm-cli/pkg/debug dependency from connection builder
- Default logger now uses glog.Level(1); callers use WithLogger() for debug logging
- Added tests for connection builder default logger behavior
- Ran go mod tidy to remove ocm-cli from go.mod

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@redhat-chai-bot
redhat-chai-bot force-pushed the ROSAENG-67544-remove-ocm-cli-dep branch from c0afff2 to 5f310e5 Compare September 23, 2026 09:30
@andreadecorte

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@rcampos2029 rcampos2029 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

/lgtm

@andreadecorte
andreadecorte merged commit 4dc1c9d into openshift-online:main Sep 23, 2026
8 checks passed
@andreadecorte

Copy link
Copy Markdown
Collaborator

Prow setup ongoing, will merge this PR manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants