Skip to content

OSAC-3975: extract shared envutil package for adapter env parsing - #288

Merged
omer-vishlitzky merged 1 commit into
osac-project:mainfrom
amito:feat/OSAC-3975-adapter-env-helpers
Aug 12, 2026
Merged

OSAC-3975: extract shared envutil package for adapter env parsing#288
omer-vishlitzky merged 1 commit into
osac-project:mainfrom
amito:feat/OSAC-3975-adapter-env-helpers

Conversation

@amito

@amito amito commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extract requireEnv, envOrDefault, readFileOrFatal, and splitAndTrim from m360-adapter into a shared adapters/envutil/ package so all adapters use the same startup helpers
  • Update both m360-adapter and echo-adapter to import envutil instead of re-implementing env parsing inline
  • Add Ginkgo test suite for the new package (9 specs)

Context

Identified during PR #236 review (OSAC-3424): m360-adapter extracted reusable helpers while echo-adapter used inline os.Getenv + if blocks. Without standardizing, each new adapter would copy one pattern or the other, compounding the inconsistency.

Changes

File Change
adapters/envutil/envutil.go New shared package: RequireEnv, EnvOrDefault, ReadFileOrFatal, SplitAndTrim
adapters/envutil/envutil_test.go Ginkgo tests (9 specs)
adapters/cmd/m360-adapter/main.go Replace local helpers with envutil.* imports, remove 37 lines
adapters/cmd/echo-adapter/main.go Replace 3 inline env-parsing blocks with envutil.*

Behavioral note

echo-adapter's KAFKA_BROKERS error message changes from "KAFKA_BROKERS is required (comma-separated broker list)" to "KAFKA_BROKERS is required" (standardized via RequireEnv).

Test plan

  • make test in adapters/ — 114 specs pass (67 framework + 15 echo + 23 m360 + 9 envutil)
  • make lint in adapters/ — 0 issues
  • No functional behavior change beyond the error message noted above

Summary by CodeRabbit

  • Improvements

    • Standardized startup configuration handling across metering adapters.
    • Added consistent support for required settings and configurable defaults.
    • Improved processing of file-based and comma-separated configuration values by trimming whitespace and ignoring empty entries.
  • Tests

    • Added coverage for environment settings, default values, file input, and value splitting.

Move requireEnv, envOrDefault, readFileOrFatal, and splitAndTrim from
m360-adapter into adapters/envutil/ so both adapters (and future ones)
share the same startup helpers instead of re-implementing them inline.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
@openshift-ci-robot

openshift-ci-robot commented Aug 12, 2026

Copy link
Copy Markdown

@amito: This pull request references OSAC-3975 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 task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

  • Extract requireEnv, envOrDefault, readFileOrFatal, and splitAndTrim from m360-adapter into a shared adapters/envutil/ package so all adapters use the same startup helpers
  • Update both m360-adapter and echo-adapter to import envutil instead of re-implementing env parsing inline
  • Add Ginkgo test suite for the new package (9 specs)

Context

Identified during PR #236 review (OSAC-3424): m360-adapter extracted reusable helpers while echo-adapter used inline os.Getenv + if blocks. Without standardizing, each new adapter would copy one pattern or the other, compounding the inconsistency.

Changes

File Change
adapters/envutil/envutil.go New shared package: RequireEnv, EnvOrDefault, ReadFileOrFatal, SplitAndTrim
adapters/envutil/envutil_test.go Ginkgo tests (9 specs)
adapters/cmd/m360-adapter/main.go Replace local helpers with envutil.* imports, remove 37 lines
adapters/cmd/echo-adapter/main.go Replace 3 inline env-parsing blocks with envutil.*

Behavioral note

echo-adapter's KAFKA_BROKERS error message changes from "KAFKA_BROKERS is required (comma-separated broker list)" to "KAFKA_BROKERS is required" (standardized via RequireEnv).

Test plan

  • make test in adapters/ — 114 specs pass (67 framework + 15 echo + 23 m360 + 9 envutil)
  • make lint in adapters/ — 0 issues
  • No functional behavior change beyond the error message noted above

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
openshift-ci Bot requested review from trewest and vladikr August 12, 2026 13:58
@openshift-ci

openshift-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amito

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 47856849-09a6-4b97-a523-c2f8424326cd

📥 Commits

Reviewing files that changed from the base of the PR and between 1bc8491 and d52f72c.

📒 Files selected for processing (4)
  • osac-metering/adapters/cmd/echo-adapter/main.go
  • osac-metering/adapters/cmd/m360-adapter/main.go
  • osac-metering/adapters/envutil/envutil.go
  • osac-metering/adapters/envutil/envutil_test.go

Walkthrough

The change adds shared startup configuration helpers and tests, then updates the Echo and M360 adapters to use them for environment variables, file reads, metrics defaults, and topic parsing.

Changes

Adapter configuration standardization

Layer / File(s) Summary
Shared configuration helpers and tests
osac-metering/adapters/envutil/*
The new envutil package provides required and optional environment lookups, trimmed file reads, and trimmed non-empty string splitting. Ginkgo tests cover these behaviors.
M360 adapter integration
osac-metering/adapters/cmd/m360-adapter/main.go
M360 now delegates environment lookup, API key file reading, metrics defaults, and topic parsing to envutil.
Echo adapter integration
osac-metering/adapters/cmd/echo-adapter/main.go
Echo now delegates required broker lookup and consumer group and metrics address defaults to envutil.

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

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the extraction of shared environment-parsing helpers into an envutil package for adapters.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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-Hardcoded-Secrets ✅ Passed The PR diff adds no literal API key, token, password, private key, credential-bearing URL, or long encoded secret; it only adds env names, defaults, and non-secret test fixtures.
No-Weak-Crypto ✅ Passed The HEAD^..HEAD diff only adds shared environment/file/string helpers and tests; it introduces no MD5, SHA1, DES, RC4, Blowfish, ECB, custom crypto, or secret comparisons.
No-Injection-Vectors ✅ Passed The PR diff adds no SQL concatenation, shell/eval/exec, pickle, unsafe YAML, os.system, or dangerouslySetInnerHTML sink; envutil only reads environment values and files.
Container-Privileges ✅ Passed The PR changes only Go source and tests; it adds no container or Kubernetes manifest privilege settings. Adapter deployment manifests remain unchanged with runAsNonRoot=true and allowPrivilegeEscal...
No-Sensitive-Data-In-Logs ✅ Passed Changed code logs only environment-variable names, file paths, configuration metadata, and counts; it does not log passwords, API keys, tokens, PII, or event payloads.
Ai-Attribution ✅ Passed The changed commit names Claude Code and includes an Assisted-by trailer plus Amit Oren's Red Hat sign-off; it has no Co-Authored-By trailer.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:59 PM UTC · Completed 2:12 PM UTC

Commit: d52f72c · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Low

  • [test-inadequate] osac-metering/adapters/envutil/envutil_test.go — The test suite only covers happy paths. RequireEnv is not tested for the fatal case (env var unset/empty), and ReadFileOrFatal is not tested for its two fatal paths (file not found, file empty after trimming).
    Remediation: Add subprocess-based tests using os/exec re-invocation pattern, or make log.Fatalf injectable via a package-level var.

  • [test-suite-organization] osac-metering/adapters/envutil/envutil_test.go:15 — Every other test package in the adapters module places the Ginkgo suite bootstrap in a dedicated *_suite_test.go file. Here, the bootstrap is inlined alongside the specs.
    Remediation: Extract the bootstrap into envutil_suite_test.go.

  • [missing-documentation] osac-metering/AGENTS.md:63 — The Key Packages table does not mention the new adapters/envutil/ shared package. Both adapters now depend on it.
    Remediation: Add a row: | adapters/envutil/ | Shared environment-variable and file-reading helpers for adapter startup |


Labels: PR extracts shared utility package from adapter code — a code-organization refactoring

import (
"os"
"path/filepath"
"testing"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] test-suite-organization

Every other test package in the adapters module places the Ginkgo suite bootstrap in a dedicated *_suite_test.go file. Here, the bootstrap is inlined alongside the specs.

Suggested fix: Extract the bootstrap into envutil_suite_test.go.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge refactoring Refactor existing code labels Aug 12, 2026
@omer-vishlitzky

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Aug 12, 2026
@omer-vishlitzky
omer-vishlitzky added this pull request to the merge queue Aug 12, 2026
Merged via the queue into osac-project:main with commit 045b7c2 Aug 12, 2026
123 of 129 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved jira/valid-reference lgtm ready-for-merge All reviewers approved — ready to merge refactoring Refactor existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants