Skip to content

Conversation

@DrAuYueng
Copy link
Contributor

@DrAuYueng DrAuYueng commented Apr 25, 2025

Description

Improve the error message of ErrorResourceAlreadyExists type to avoid losing the original Err.

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

NONE

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

Signed-off-by: DrAuYueng <ouyang1204@gmail.com>
@DrAuYueng DrAuYueng requested a review from a team as a code owner April 25, 2025 10:48
@marvin-tigera marvin-tigera added this to the Calico v3.31.0 milestone Apr 25, 2025
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Apr 25, 2025
@CLAassistant
Copy link

CLAassistant commented Apr 25, 2025

CLA assistant check
All committers have signed the CLA.

@caseydavenport
Copy link
Member

@DrAuYueng Thanks for the PR - before we can merge you'll need to sign the CLA to make the bot happy.

Is this in service of some other goal? Or just something you noticed? I would expect this error to always be the same thing, so not especially useful to log out.

@DrAuYueng
Copy link
Contributor Author

@caseydavenport IMO, Err is defined and should not be swallowed in the subsequent processing, so err seems meaningless, for example:

if b.Allocations[ordinal] != nil {
		return cerrors.ErrorResourceAlreadyExists{
			Err:        fmt.Errorf("Address already assigned in block"),
			Identifier: address.String(),
		}
}

@caseydavenport
Copy link
Member

/sem-approve

@caseydavenport caseydavenport added docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact labels May 5, 2025
@marvin-tigera marvin-tigera removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels May 5, 2025
@caseydavenport caseydavenport added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented merge-when-ready labels May 5, 2025
@marvin-tigera marvin-tigera removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels May 5, 2025
@caseydavenport
Copy link
Member

Makes sense, thanks @DrAuYueng

Signed-off-by: DrAuYueng <ouyang1204@gmail.com>
@marvin-tigera
Copy link
Contributor

Removing "merge-when-ready" label due to new commits

@DrAuYueng
Copy link
Contributor Author

@caseydavenport CI check failed due to error judgment problem. I just fixed it. Please take a look again. Thanks.

@caseydavenport
Copy link
Member

/sem-approve

@marvin-tigera marvin-tigera merged commit f1ef187 into projectcalico:master Sep 15, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required Docs not required for this change merge-when-ready release-note-not-required Change has no user-facing impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants