Skip to content

cli-plugins/manager: IsNotFound: don't depend on causer interface - #5794

Closed
thaJeztah wants to merge 1 commit into
docker:masterfrom
thaJeztah:no_causer
Closed

cli-plugins/manager: IsNotFound: don't depend on causer interface#5794
thaJeztah wants to merge 1 commit into
docker:masterfrom
thaJeztah:no_causer

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztahthaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Feb 4, 2025
@thaJeztahthaJeztah added this to the 28.0.0 milestone Feb 4, 2025
@thaJeztahthaJeztah self-assigned this Feb 4, 2025
@codecov-commenter

codecov-commenter commented Feb 4, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.17%. Comparing base (ce30966) to head (7f8c43e).
Report is 21 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #5794 +/- ##
==========================================
- Coverage 59.44% 59.17% -0.27% 
==========================================
Files 347 353 +6 Lines 29394 29472 +78 ==========================================
- Hits 17472 17440 -32 - Misses 10950 11059 +109 - Partials 972 973 +1 

@thaJeztah
thaJeztah requested a review from a teamFebruary 4, 2025 17:24
err = e.Cause()
var e *pluginError
if errors.As(err, &e) {
err = e.Unwrap()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you be able to add more context to the commit on why this needs to change?
The error could also be wrapped multiple times before it's a notFound type. Are we sure we never wrap the error more than once?

@thaJeztahthaJeztah modified the milestone: 28.0.0Feb 11, 2025

@BenehikoBenehiko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can also get this one merged soon if you'd like. I don't have the full context yet though (see previous comment), but the change seems alright.

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Oh! Sorry forgot about this one; I need to double-check if this could cause a change in behavior; considering that errors.As does a deep-first match, which could result in some different semantics; ran into that when looking at using the containerd errdefs package; moby/moby#49378 (comment)

@thaJeztah
thaJeztah marked this pull request as draft February 21, 2025 09:12
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@thaJeztah
thaJeztah deleted the no_causer branch September 30, 2025 10:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactorPR's that refactor, or clean-up codestatus/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah@codecov-commenter@Benehiko