-
Notifications
You must be signed in to change notification settings - Fork 122
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
update in kubervirt rawconfig #1143
update in kubervirt rawconfig #1143
Conversation
/retest |
@sankalp-r IMO this changeset introduces breaking changes for the existing kubevirt machine-deployments. I think we need to consider that as well, wdyt? |
@ahmedwaleedmalik Yes these changes introduces breaking changes. |
/retest |
1 similar comment
/retest |
/retest |
@sankalp-r What is the status of this PR ? |
As the UI for this change is not ready yet, can't proceed ahead. This change needs UI changes as well, mentioned by Marcin: |
Labels: map[string]string{ | ||
"kubevirt.io/vm": machine.Name, | ||
"kubevirt.io/vm": c.VirtualMachineName, |
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 are missing the labels coming from the Preset here (from line 372).
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.
Preset labels are only required to be added in VirtualMachineInstanceTemplateSpec
(line 414)
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.
perfect.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
/lgtm |
LGTM label has been added. Git tree hash: f613de30201f084e7594618fd57bebad56834b51
|
/retest |
Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>
/hold cancel |
/hold this PR will be merged after the release of machine controller. |
DNSPolicy corev1.DNSPolicy | ||
CPUs string | ||
Memory string | ||
Namespace string |
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.
As we will merge #1204 in the same release, we should now remove the Namespace
.
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 find it confusing to keep the Namespace
here as it's not a configurable option any more..
DNSPolicy corev1.DNSPolicy | ||
CPUs string | ||
Memory string | ||
Namespace string |
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 find it confusing to keep the Namespace
here as it's not a configurable option any more..
Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>
Labels: map[string]string{ | ||
"kubevirt.io/vm": machine.Name, | ||
"kubevirt.io/vm": c.VirtualMachineName, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>
// PrimaryDisk | ||
type PrimaryDisk struct { | ||
Disk | ||
OsImage providerconfigtypes.ConfigVarString `json:"osImage,omitempty"` |
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.
Do we want to have here the same struct as in provider.go ?
/lgtm |
LGTM label has been added. Git tree hash: a936d56e7463d42d73f9a35493c1556416553140
|
/hold cancel |
/retest |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: moadqassem, sankalp-r 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 |
* update in kubervirt rawconfig Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com> * rawConfig field renaming Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com> * update kubevirt provider Config Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com> * add namespace option in CloudConfig Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com> * create VM in dedicated ns Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com> * remove vmName from rawConfig Signed-off-by: Sankalp Rangare <sankalprangare786@gmail.com>
Signed-off-by: Sankalp Rangare sankalprangare786@gmail.com
What this PR does / why we need it:
This PR updates kubeVirt's rawConfig and creation of VM using presets.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>
format, will close the issue(s) when PR gets merged):Fixes #1113 , kubermatic/kubermatic#9108, kubermatic/kubermatic#9095
Special notes for your reviewer:
Optional Release Note: