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 1885319: Revise DataVolumeTemplate API field #6846
Bug 1885319: Revise DataVolumeTemplate API field #6846
Conversation
@yaacov: This pull request references Bugzilla bug 1885319, 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. |
/hold freeze 🏂 cc:// @davidvossel |
/hold cancel |
let dataVolume = dataVolumeTemplates.find((dv) => getName(dv) === volume.dataVolume.name); | ||
let dataVolume = dataVolumeTemplates.find( | ||
(dv) => getName(dv) === volume.dataVolume.name, | ||
) as V1alpha1DataVolume; |
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.
you dont have to typecast here and anywhere else. Just specify the correct types from the beginning
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.
fixed 👍
0e34f1c
to
c98be22
Compare
/lgtm |
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.
also what about
- tests ?
- clone-vm ?
@@ -221,7 +230,9 @@ export class VMWrapper extends K8sResourceWrapper<VMKind, VMWrapper> implements | |||
storages.map((storage) => storage.disk), | |||
); | |||
this.data.spec.template.spec.volumes = _.compact(storages.map((storage) => storage.volume)); | |||
this.data.spec.dataVolumeTemplates = _.compact(storages.map((storage) => storage.dataVolume)); | |||
this.data.spec.dataVolumeTemplates = _.compact( |
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.
we should be consistent with this approach here - pleas update prependStorage
as well
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.
done
@@ -33,6 +34,14 @@ import { VirtualMachineImportModel, VirtualMachineModel } from '../../../models' | |||
import { buildOwnerReferenceForModel } from '../../../utils'; | |||
import { transformDevices } from '../../../selectors/vm/devices'; | |||
|
|||
export const getDataVolumeTemplateSpec = (dv: V1alpha1DataVolume): V1DataVolumeTemplateSpec => |
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.
what about
export const getDataVolumeTemplateSpec = (dv: V1alpha1DataVolume): V1DataVolumeTemplateSpec => | |
export const convertToDataVolumeTemplateSpec = (dv: V1alpha1DataVolume): V1DataVolumeTemplateSpec => |
or something similar to this?
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.
also it probably shouldn't reside in vm-wrapper
/hold |
/retest |
c98be22
to
3e9d834
Compare
8178aa3
to
672fc44
Compare
size: getPvcStorageSize(pvc), | ||
storageClassName: getPvcStorageClassName(pvc), | ||
}).build(); | ||
const clonedDVTemplate: V1DataVolumeTemplateSpec = { |
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.
can we rather use a DataVolumeWrapper here for easier use?
We could add a special method there similar to asResource for compatibility with V1DataVolumeTemplateSpec
size: getDataVolumeStorageSize(dataVolume), | ||
storageClassName: getDataVolumeStorageClassName(dataVolume), | ||
}).build(); | ||
const clonedDVTemplate: V1DataVolumeTemplateSpec = { |
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.
similar here
/retest |
2 similar comments
/retest |
/retest |
@yaacov: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/retest |
1 similar comment
/retest |
/hold cancel |
dd4f0be
to
7b00143
Compare
force push to skip buggy CI, @metalice please re-review |
/retest |
3 similar comments
/retest |
/retest |
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metalice, rawagner, yaacov 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
1 similar comment
/retest |
@yaacov: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/retest |
@yaacov: All pull requests linked via external trackers have merged: Bugzilla bug 1885319 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. |
The vm.Spec.DataVolumeTemplate api list actually contains DataVolumes rather than a struct that restricts the fields to only the metadata and spec. This gives the user the impression they can set things like the DataVolume group/kind and status when they in fact cannot.
Refs:
kubevirt/kubevirt#4261
kubevirt/kubevirt#4319
Screeshots:
The Bug (using patch 4216):
Signed-off-by: yaacov kobi.zamir@gmail.com