Skip to content

Export pod inspect functions for external consumption#2308

Open
kaovilai wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:export-inspect-pod-functions
Open

Export pod inspect functions for external consumption#2308
kaovilai wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:export-inspect-pod-functions

Conversation

@kaovilai

@kaovilai kaovilai commented Jul 9, 2026

Copy link
Copy Markdown
Member

Export the following unexported functions in pkg/cli/admin/inspect/pod.go so they can be imported by external Go packages without vendoring the full oc binary (which pulls in CVE-affected transitive dependencies):

  • gatherPodData → GatherPodData
  • gatherContainerInfo → GatherContainerInfo
  • gatherContainerAllLogs → GatherContainerAllLogs
  • gatherContainerLogs → GatherContainerLogs
  • gatherContainerRotatedLogFiles → GatherContainerRotatedLogFiles
  • filterContainerLogsErrors → FilterContainerLogsErrors
  • rotatedLogFilename → RotatedLogFilename

All internal callers in pod.go and namespace.go are updated to use the new exported names. Godoc comments are added to each exported function per project conventions. No behavioral changes.

Summary by CodeRabbit

  • New Features
    • Expanded inspection coverage to collect data from both regular and init containers.
    • Added support for gathering current, previous, and rotated container logs more consistently.
  • Bug Fixes
    • Improved log collection error handling so expected missing-previous-log messages are filtered out, reducing noisy failures.

Export the following unexported functions in pkg/cli/admin/inspect/pod.go
so they can be imported by external Go packages without vendoring the
full oc binary (which pulls in CVE-affected transitive dependencies):

- gatherPodData → GatherPodData
- gatherContainerInfo → GatherContainerInfo
- gatherContainerAllLogs → GatherContainerAllLogs
- gatherContainerLogs → GatherContainerLogs
- gatherContainerRotatedLogFiles → GatherContainerRotatedLogFiles
- filterContainerLogsErrors → FilterContainerLogsErrors
- rotatedLogFilename → RotatedLogFilename

All internal callers in pod.go and namespace.go are updated to use the
new exported names. Godoc comments are added to each exported function
per project conventions. No behavioral changes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@openshift-ci
openshift-ci Bot requested review from ardaguclu and ingvagabund July 9, 2026 19:35
@openshift-ci

openshift-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kaovilai
Once this PR has been reviewed and has the lgtm label, please assign atiratree for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 Jul 9, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 8a389e81-732e-442f-97de-f4fefe66511b

📥 Commits

Reviewing files that changed from the base of the PR and between 22c69c0 and 0dbeb28.

📒 Files selected for processing (2)
  • pkg/cli/admin/inspect/namespace.go
  • pkg/cli/admin/inspect/pod.go

Walkthrough

Several unexported helper methods and functions in pkg/cli/admin/inspect/pod.go for gathering pod and container data/logs are renamed to exported equivalents, and the caller in namespace.go is updated to use the new exported method name.

Changes

Export Inspect Pod/Container Helpers

Layer / File(s) Summary
Exported pod/container gathering APIs
pkg/cli/admin/inspect/pod.go, pkg/cli/admin/inspect/namespace.go
Renames gatherPodData, gatherContainerInfo, gatherContainerAllLogs, filterContainerLogsErrors, rotatedLogFilename, gatherContainerRotatedLogFiles, and gatherContainerLogs to exported GatherPodData, GatherContainerInfo, GatherContainerAllLogs, FilterContainerLogsErrors, RotatedLogFilename, GatherContainerRotatedLogFiles, and GatherContainerLogs respectively, preserving existing logic; updates the namespace.go call site to invoke GatherPodData.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: exporting pod inspect helper functions for external use.
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.
Stable And Deterministic Test Names ✅ Passed The PR only changes pkg/cli/admin/inspect non-test files; no Ginkgo titles were added or modified, so no unstable test names are introduced.
Test Structure And Quality ✅ Passed No Ginkgo tests were added or modified in this PR; only production code in pod.go/namespace.go changed, so the checklist is not applicable.
Microshift Test Compatibility ✅ Passed PR only exports inspect helpers in two non-test files; no Ginkgo tests or MicroShift-sensitive APIs were added.
Single Node Openshift (Sno) Test Compatibility ✅ Passed Diff only exports inspect helpers in non-test files; no new Ginkgo e2e tests or SNO-sensitive assertions were added.
Topology-Aware Scheduling Compatibility ✅ Passed Only CLI inspect helper exports changed in pod.go/namespace.go; no manifests, controllers, node selectors, affinities, or topology-aware scheduling logic were added.
Ote Binary Stdout Contract ✅ Passed Diff only exports helper functions; no main/init/TestMain/setup code or stdout writes were added in the touched files.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed Only implementation files changed; no new Ginkgo tests or network-relevant test code were added.
No-Weak-Crypto ✅ Passed Touched files only export/rename inspect helpers; no weak-crypto algorithms, custom crypto, or secret comparisons appear in the diff.
Container-Privileges ✅ Passed Diff only exports inspect helpers and updates callers; no container/K8s manifests or securityContext fields were added or changed.
No-Sensitive-Data-In-Logs ✅ Passed No new logging was added; existing Infof calls only emit namespace/pod names and benign errors, not secrets or PII.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@kaovilai

Copy link
Copy Markdown
Member Author

/retest

@ardaguclu

Copy link
Copy Markdown
Member

Exporting those functions means that we should provide backwards compatibility guarantees for anyone using them. We wouldn't not want to expose those functions, since they can likely change.

However, I understand the inconvenience here. If you only depend on these functions, why don't you directly copy to your repository and fully drop the dependency from oc?. That would be easier.
/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2026
@kaovilai

Copy link
Copy Markdown
Member Author

/retest

@kaovilai

Copy link
Copy Markdown
Member Author

Exporting those functions means that we should provide backwards compatibility guarantees for anyone using them. We wouldn't not want to expose those functions, since they can likely change.

We don't require backwards compatibility guarantees. If we do we'd use older go.mod

@kaovilai

Copy link
Copy Markdown
Member Author

/retest

@kaovilai

Copy link
Copy Markdown
Member Author

why don't you directly copy to your repository and fully drop the dependency from oc?. That would be easier.

We want future compatibility updates to flow in without recopying in the future. If things break for older version then we pin for release branch for older releases.

@ardaguclu

Copy link
Copy Markdown
Member

Exporting those functions means that we should provide backwards compatibility guarantees for anyone using them. We wouldn't not want to expose those functions, since they can likely change.

We don't require backwards compatibility guarantees. If we do we'd use older go.mod

I was referring to the backwards compatibility guarantees of oc.

@openshift-ci

openshift-ci Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@kaovilai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-scos-images 0dbeb28 link true /test okd-scos-images

Full PR test history. Your PR dashboard.

Details

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.

@kaovilai

Copy link
Copy Markdown
Member Author

I understand the compatibility concern, but I don’t think this PR materially changes oc’s existing compatibility burden.

These functions already exist and are already part of the inspect implementation. The PR does not introduce new behavior, new flags, new output, or a new CLI contract; it only allows downstream Go consumers to call the same implementation instead of copying it. If these helpers need to change in the future, downstreams can pin to an older github.com/openshift/oc revision for older release branches and update intentionally when moving forward.

Copying the code would solve the immediate compile problem, but it creates a worse long-term maintenance model: downstream copies drift from upstream, miss fixes, and need manual re-syncing. Exporting the helpers keeps the implementation centralized and lets future inspect fixes flow naturally.

If the exported names are the issue, I’m happy to add wording to the GoDoc making the intended support level clear. But as-is, this PR has essentially no downside for current oc users and gives downstream consumers a cleaner, less divergent integration path.

@tchap

tchap commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

There is a difference between locking behavior and locking package API, but I guess the additional burden is not big.

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

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants