Bug 2182938: clone revisions alongside vm #1224
Conversation
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is invalid:
Comment 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. |
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is invalid:
Comment 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. |
upalatucci
force-pushed
the
clone-vm-instancetype
branch
2 times, most recently
from
510ff06
to
0a51d16
Compare
|
/bugzilla refresh |
|
@avivtur: This pull request references Bugzilla bug 2182938, 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
Requesting review from QA contact: 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. |
There was a problem hiding this comment.
This will pull down the latest revision of the instance type or preference and stash that in a fresh controller revision. This could easily cause the VirtualMachine to change as a result so I'd highly recommend cloning the referenced controller revisions into the new namespace instead.
|
@lyarwood: changing LGTM is restricted to collaborators 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. |
|
/hold as per Lee's comment |
I should add that the OwnerReference will need to be updated to point to the new VirtualMachine in the new namespace. |
|
/unhold |
upalatucci
force-pushed
the
clone-vm-instancetype
branch
from
0e0d3a7
to
475ae90
Compare
upalatucci
changed the title
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
upalatucci
force-pushed
the
clone-vm-instancetype
branch
from
475ae90
to
a8c942b
Compare
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
2 similar comments
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@upalatucci: This pull request references Bugzilla bug 2182938, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
| }); | ||
| } catch (error) { | ||
| if (error.code !== 404) { | ||
| throw error; |
There was a problem hiding this comment.
You are throwing exceptions but not handling them on the other files when you use it...
I would have handled it here and reduced the possibility someone won't add a try-catch block or concating a .catch
There was a problem hiding this comment.
The error would be caught by TabModal and it would display the error with the nice UI.
There was a problem hiding this comment.
Then why limit the error only to not found? why add a try a catch at all if tabmodal handles it?
There was a problem hiding this comment.
here I'm trying to fetch a ControllerVersion in another namespace using the referenceName.
I'll create it only if it's not there.
The fact that k8sGet throws a 404 error, its not actually an error. It just means that does not exist in that namespace.
There was a problem hiding this comment.
Probably in the next version, the backend will handle cloning in different namespaces and we'll remove all the logic. But for now we need that
There was a problem hiding this comment.
im sorry, but it isn't making sense to me. Please clarify in slack, thank you :)
upalatucci
force-pushed
the
clone-vm-instancetype
branch
from
a8c942b
to
25bc104
Compare
upalatucci
force-pushed
the
clone-vm-instancetype
branch
from
25bc104
to
f29efb6
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avivtur, metalice, upalatucci 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 |
|
@upalatucci: All pull requests linked via external trackers have merged: Bugzilla bug 2182938 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. |
|
/cherry-pick release-4.13 |
|
@gouyang: new pull request created: #1294 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. |
|
/cherry-pick release-4.13 |
|
@upalatucci: new pull request could not be created: failed to create pull request against kubevirt-ui/kubevirt-plugin#release-4.13 from head openshift-cherrypick-robot:cherry-pick-1224-to-release-4.13: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for openshift-cherrypick-robot:cherry-pick-1224-to-release-4.13."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"} 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. |
📝 Description
Clone
ControllerRevisionforinstancetypeandpreferencein the new namespace as suggested by @lyarwoodThe flow
(Only In case of cloning vm created through instance type)
if in the destination namespace there is no ControllerRevision with the same name, we'll create it. (for instance type and prefernece)
clone the vm
in case the controller revision was not created, the cloned vm will be added as owner. In case the controlelr revision was created, does not have owner and we'll add the cloned vm as only owner