Skip to content

CLID-657: Add warning when using different version for m2d and d2m#1444

Open
adolfo-ab wants to merge 1 commit into
openshift:mainfrom
adolfo-ab:CLID-657
Open

CLID-657: Add warning when using different version for m2d and d2m#1444
adolfo-ab wants to merge 1 commit into
openshift:mainfrom
adolfo-ab:CLID-657

Conversation

@adolfo-ab

@adolfo-ab adolfo-ab commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Description

When users do mirror-to-disk using a given version of oc-mirror, and then disk-to-mirror using a different version, they might find unexpected behaviors if major changes have occurred between versions.

With this PR I propose saving the version metadata during m2d (in working-dir/internal/oc-mirror-version.json), and then compare it with the version of the binary during d2m. If there is a mismatch or the version metadata file cannot be found, we show a non-fatal warning. For now we compare the GitVersion and GitCommit, but when we are removed from the payload we can consider using the actual version of oc-mirror.

Github / Jira issue:
CLID-657

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code Improvements (Refactoring, Performance, CI upgrades, etc)
  • Internal repo assets (diagrams / docs on github repo)
  • This change requires a documentation update on openshift docs

How Has This Been Tested?

  • Running m2d from main, then d2m from this branch -> missing file warning
  • Running m2d from this branch, change the version metadata file, then run d2m -> version mismatch warning
  • Same thing with a malformed json -> malformed file warning

Summary by CodeRabbit

  • New Features
    • Added persistent version metadata to mirror-to-disk workflows to improve build provenance tracking.
    • When converting disk mirrors back to local registries, the tool now validates archived metadata against the current build (version/commit).
  • Bug Fixes
    • Metadata write failures during mirror-to-disk now emit a warning instead of stopping the command.
  • Tests
    • Added coverage for metadata write/read round-trips and validation behavior, including missing, mismatch, corrupt, and error scenarios.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 16, 2026
@openshift-ci-robot

openshift-ci-robot commented Jun 16, 2026

Copy link
Copy Markdown

@adolfo-ab: This pull request references CLID-657 which is a valid jira issue.

Details

In response to this:

Description

When users do mirror-to-disk using a given version of oc-mirror, and then disk-to-mirror using a different version, they might find unexpected behaviors if major changes have occurred between versions.

With this PR I propose saving the version metadata during m2d (in working-dir/internal/oc-mirror-version.json), and then compare it with the version of the binary during d2m. If there is a mismatch or the version metadata file cannot be found, we show a non-fatal warning. For now we compare the GitVersion and GitCommit, but when we are removed from the payload we can consider using the actual version of oc-mirror.

Github / Jira issue:
CLID-657

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code Improvements (Refactoring, Performance, CI upgrades, etc)
  • Internal repo assets (diagrams / docs on github repo)
  • This change requires a documentation update on openshift docs

How Has This Been Tested?

  • Running m2d from main, then d2m from this branch -> missing file warning
  • Running m2d from this branch, change the version metadata file, then run d2m -> version mismatch warning
  • Same thing with a malformed json -> malformed file warning

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.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Caution

Review failed

An error occurred during the review process. Please try again later.

Walkthrough

Adds oc-mirror version metadata persistence and validation to mirror-to-disk and disk-to-mirror workflows.

Changes

Version Metadata Tracking

Layer / File(s) Summary
Metadata contract and helpers
internal/pkg/config/metadata.go, internal/pkg/version/metadata.go
Adds the version metadata file/path constants, defines VersionMetadata with GitVersion and GitCommit, and implements write/read/check helpers for the metadata file.
Metadata tests
internal/pkg/version/metadata_test.go
Adds tests for metadata round-tripping, mismatch and missing-metadata warnings, corrupt JSON handling, and missing-file errors.
Executor integration
internal/pkg/cli/executor.go
Writes version metadata during mirror-to-disk execution and checks metadata during disk-to-mirror execution, logging warnings when needed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error The new warnings log raw path/error/metadata strings from untrusted inputs, so malformed archives or crafted paths could leak sensitive text into logs. Sanitize or structure these log fields before logging; avoid emitting raw error/warning strings and archive metadata directly.
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (13 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the new version-mismatch warning added between m2d and d2m workflows.
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 All added test titles are static and descriptive; none include generated or run-specific values.
Test Structure And Quality ✅ Passed PASS: These are plain Go unit tests, not Ginkgo; they use t.TempDir, focused subtests, no cluster waits, and match existing require-based patterns.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the new test file is a plain Go unit-test suite with Test* functions and no unsupported OpenShift APIs.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS — The added test file uses plain testing Test* functions, not Ginkgo e2e specs, so no SNO multi-node assumptions are introduced.
Topology-Aware Scheduling Compatibility ✅ Passed Only CLI/config/version metadata files changed; no deployments, controllers, or scheduling constraints were added.
Ote Binary Stdout Contract ✅ Passed The PR only adds version-metadata helpers and non-entrypoint warnings; no new stdout writes or logging-setup changes in main/init/TestMain paths.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the new tests are local unit tests with temp dirs/JSON only, and they don't assume IPv4 or external connectivity.
No-Weak-Crypto ✅ Passed No MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret/token comparisons were added; only version string/commit equality checks exist.
Container-Privileges ✅ Passed The PR only adds version metadata helpers and logging checks; it does not modify any privileged/container security settings or manifests.
✨ Finishing Touches
🧪 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.

@openshift-ci openshift-ci Bot requested review from aguidirh and r4f4 June 16, 2026 09:42

@coderabbitai coderabbitai Bot 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.

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 `@internal/pkg/cli/executor.go`:
- Around line 918-920: The version metadata write error in the
version.WriteVersionMetadata call is being treated as a fatal error that causes
the mirror-to-disk operation to fail completely. Instead of returning an error,
log this as a non-fatal warning to allow the mirror-to-disk operation to
continue successfully, consistent with how similar metadata write failures are
handled in the disk-to-mirror code path. Capture the error from
version.WriteVersionMetadata and pass it to a logging function (e.g., a warning
or info log) rather than wrapping it in a fmt.Errorf return statement.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 998fda2d-0b81-4f19-a5e8-4198e8f31c39

📥 Commits

Reviewing files that changed from the base of the PR and between ef54cec and 8b43eed.

📒 Files selected for processing (4)
  • internal/pkg/cli/executor.go
  • internal/pkg/config/metadata.go
  • internal/pkg/version/metadata.go
  • internal/pkg/version/metadata_test.go

Comment thread internal/pkg/cli/executor.go
@adolfo-ab adolfo-ab force-pushed the CLID-657 branch 2 times, most recently from 12fd44e to 0d27216 Compare June 16, 2026 09:59

@aguidirh aguidirh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR Adolfo.

I did few suggestions, please let me know if you have questions about them.

Comment thread internal/pkg/version/metadata.go Outdated
Comment thread internal/pkg/version/metadata.go Outdated
@adolfo-ab

Copy link
Copy Markdown
Contributor Author

/test v1-e2e

@openshift-ci

openshift-ci Bot commented Jun 18, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adolfo-ab, aguidirh

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2026
Comment thread internal/pkg/version/metadata.go Outdated
Comment thread internal/pkg/version/metadata.go Outdated
Comment thread internal/pkg/cli/executor.go
Comment thread internal/pkg/cli/executor.go
Comment thread internal/pkg/version/metadata.go Outdated

@r4f4 r4f4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2026
@openshift-ci

openshift-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown

@adolfo-ab: all tests passed!

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.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants