New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1904385: make ControllerPublishVolume idempotent #63
Bug 1904385: make ControllerPublishVolume idempotent #63
Conversation
/retest |
/retest |
/test e2e-ovirt |
1 similar comment
/test e2e-ovirt |
111e501
to
cc31f3f
Compare
/test e2e-ovirt |
@bennyz: This pull request references Bugzilla bug 1904385, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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/test-infra repository. |
pkg/service/controller.go
Outdated
_, err = diskAttachmentByVmAndDisk(conn, req.NodeId, req.VolumeId) | ||
if err != nil { | ||
// If attachment was not found we are good to | ||
_, ok := err.(*AttachmentNotFoundError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to use errors.Is()
or errors.As()
here because it checks wrapped errors too. Not an issue now, but later with some refactoring this could become relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I tried it previously, but I think it doesn't apply here since the error isn't wrapped (and there is no need to wrap it as this error is actually desired in this case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more of a nitpick, really. I'm just thinking about the future when somebody is working on this code. If the error will be wrapped in the future using the errors
methods make the code more resilient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with nits :) do you have an example of how it would look in this case? Because as I understand to use errors
I'd need to create an instance of the error struct, but perhaps I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From memory it should work like this:
notFoundError := &AttachmentNotFoundError{}
if errors.As(err, ¬FoundError) {
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, like I said, it requires an additional instance which looks weird, but I changed it anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you can also store the error in a variable and use errors.Is
, that is less quirky.
cc31f3f
to
2616f4e
Compare
/hold @janoszen can you lgtm? |
/lgtm |
/test e2e-ovirt |
If the disk is already attached to the VM, return OK Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
2616f4e
to
e7fd437
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bennyz, janoszen 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:
Approvers can indicate their approval by writing |
I forgot a |
/unhold |
@bennyz: All pull requests linked via external trackers have merged: Bugzilla bug 1904385 has been moved to the MODIFIED state. In response to this:
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/test-infra repository. |
If the disk is already attached to the VM, return OK
Signed-off-by: Benny Zlotnik bzlotnik@redhat.com