Reused errdefs define error#7871
Conversation
|
Hi @yanggangtony. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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/test-infra repository. |
yanggangtony
force-pushed
the
error-def
branch
from
e64c884 to
cfa523c
Compare
| var ( | ||
| // ErrNotFound represents an error returned when object not found in meta store | ||
| ErrNotFound = errors.New("not found") | ||
| // ErrAlreadyExists represents an error returned when object can't be duplicated in meta store | ||
| ErrAlreadyExists = errors.New("object already exists") | ||
| ) |
There was a problem hiding this comment.
Actually, one comment. There's a small chance, but just to be safe in case anyone was relying on these in some package, could you just assign these to errdefs.ErrNotFound and errdefs.ErrNotImplemented
There was a problem hiding this comment.
@dcantah
Thanks for review that.
I think you point is good.
I had updated it..
thanks for that..
There was a problem hiding this comment.
We do not guarantee the Go API stability, and it is unlikely that somebody is depending on these variables. Also it is quite easy for such potential users to apply this change.
Signed-off-by: yanggang <[email protected]>
yanggangtony
force-pushed
the
error-def
branch
from
cfa523c to
b10536d
Compare
| // ErrAlreadyExists represents an error returned when object can't be duplicated in meta store | ||
| ErrAlreadyExists = errors.New("object already exists") | ||
| ErrAlreadyExists = errdefs.ErrAlreadyExists | ||
| ) |
There was a problem hiding this comment.
Can we just remove these variables from the snapshots/devmapper pkg?
There was a problem hiding this comment.
The author did actually in the first iteration, to be safe I said maybe we can just assign them to the errdefs equivalents. I think it's probably fine to get rid of though, we don't have any callers here that check for these, was mostly thinking external, but not sure what someone would even be doing calling things from snapshotter/devmapper
There was a problem hiding this comment.
I've seen folks rely on some pretty strange implementation details, so I always err on the side of safety 🤣
There was a problem hiding this comment.
😋😋
for now, containerd 1.x , keep compatible is good.
Maybe we can delete these dead code in 2.x?
4 participants
Signed-off-by: yanggang [email protected]
Reused define error in package 'errdefs' , like
errdefs.ErrNotFound and errdefs.ErrAlreadyExists, errdefs.ErrNotImplemented