OCPBUGS-63189: Unify ErrAlreadyExists to correctly match

[ardaguclu]

When the uploaded blob is already existed, S3 raises ErrAlreadyExists error correctly. However, mirror.go compares the error against duplicate of ErrAlreadyExists error. So it mismatches.

This PR removes duplicate and unused ErrAlreadyExists in mappings.go and correctly compare the error in S3.

[openshift-ci-robot]

@ardaguclu: This pull request references Jira Issue OCPBUGS-63189, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.21.0) matches configured target version for branch (4.21.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @zhouying7780

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

When the uploaded blob is already existed, S3 raises ErrAlreadyExists error correctly. However, mirror.go compares the error against duplicate of ErrAlreadyExists error. So it mismatches.

This PR removes duplicate and unused ErrAlreadyExists in mappings.go and correctly compare the error in S3.

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]

Walkthrough

A public error variable ErrAlreadyExists is removed from mappings.go, and the corresponding error handling in mirror.go is refactored to use errors.Is() with imagesource.ErrAlreadyExists instead of direct equality comparison.

Changes

Cohort / File(s)	Summary
Error handling consolidation pkg/cli/image/mirror/mappings.go, pkg/cli/image/mirror/mirror.go	Removed local ErrAlreadyExists variable definition; updated copyBlob function to use errors.Is(err, imagesource.ErrAlreadyExists) for error comparison, enabling detection of wrapped or nested errors in the error chain.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Verify that imagesource.ErrAlreadyExists exists and is the intended replacement error
• Confirm errors.Is() semantics preserve the original error-handling logic in copyBlob
• Check for any other references to the removed ErrAlreadyExists variable that may cause compilation failures

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 8642d88 and d572614.

📒 Files selected for processing (2)

• pkg/cli/image/mirror/mappings.go (0 hunks)
• pkg/cli/image/mirror/mirror.go (1 hunks)

💤 Files with no reviewable changes (1)

• pkg/cli/image/mirror/mappings.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/cli/image/mirror/mirror.go

🔇 Additional comments (1)

pkg/cli/image/mirror/mirror.go (1)

723-741: I'm unable to complete the verification of this review comment because the repository failed to clone in the sandbox environment. This prevents me from:

1. Examining the actual code changes at lines 723-741 in pkg/cli/image/mirror/mirror.go
2. Searching for all ErrAlreadyExists references to confirm the unification claim
3. Verifying that the change correctly uses errors.Is() for wrapped error handling
4. Confirming whether a local duplicate was indeed removed

Without access to the codebase, I cannot verify the technical claims made in the original review comment about the error handling changes and their consistency across the codebase.

Unifying ErrAlreadyExists handling with errors.Is appears sound in principle

Switching from direct equality comparison to errors.Is(err, imagesource.ErrAlreadyExists) would correctly handle wrapped errors from the S3 backend, treating them as no-ops consistent with the canonical sentinel. The surrounding error-handling control flow (mount handling and generic error returns) remains stable.

However, the specific claims about unified error handling and removal of local duplicates could not be verified due to repository access limitations.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[tchap]

/lgtm

Would be lovely to have tests for this, though.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, tchap

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ardaguclu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ardaguclu]

/verified by pass
It is a minor and relatively safe change. We can trust CI results.

[openshift-ci-robot]

@ardaguclu: This PR has been marked as verified by pass.

In response to this:

/verified by pass
It is a minor and relatively safe change. We can trust CI results.

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-robot]

/retest-required

Remaining retests: 0 against base HEAD 8642d88 and 2 for PR HEAD d572614 in total

[ardaguclu]

/retest

[ardaguclu]

/retest

[openshift-ci]

@ardaguclu: all tests passed!

Full PR test history. Your PR dashboard.

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.

[openshift-ci-robot]

@ardaguclu: Jira Issue Verification Checks: Jira Issue OCPBUGS-63189
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-63189 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

In response to this:

When the uploaded blob is already existed, S3 raises ErrAlreadyExists error correctly. However, mirror.go compares the error against duplicate of ErrAlreadyExists error. So it mismatches.

This PR removes duplicate and unused ErrAlreadyExists in mappings.go and correctly compare the error in S3.

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.