-
Notifications
You must be signed in to change notification settings - Fork 667
KNIP-525: Add an Environment tab for Virtual Machines #4791
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
KNIP-525: Add an Environment tab for Virtual Machines #4791
Conversation
297a156 to
fbd87f6
Compare
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/selectors.ts
Outdated
Show resolved
Hide resolved
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.
why not getRandomChars(16) ? what am I missing ?
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.
because the limit is 10.
if you give it 16 you will get 10 eventually.
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.
kubevirt-create-nic-modal__buttons ?
44c356a to
39033e8
Compare
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/selectors.ts
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/selectors.ts
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/vm-environment-page.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/vm-environment-page.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/vm-environment-page.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/vm-environment-page.tsx
Outdated
Show resolved
Hide resolved
99fa6cb to
c660810
Compare
830449b to
2f3f105
Compare
|
/retest |
815fc8f to
5bb0ed6
Compare
frontend/public/module/k8s/index.ts
Outdated
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 can further simplify and merge types ConfigMapListKind, SecretListKind and ServiceAccountListKind using generics
export type ListKind<R extends K8sResourceCommon> = K8sResourceCommon & {
items: R[];
}
and the usage for example here https://github.com/openshift/console/blob/5bb0ed6137a5d661ab98118cee25e1da444cb8f1/frontend/packages/kubevirt-plugin/src/components/vms/vm-environment/selectors.ts#L63 would be
): ListKind<ConfigMapKind | SecretKind | ServiceAccountKind> => {
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 shoud define new type - something like
type EnvDisk = [string, EnvVarSource, number];
| export const getSerial = (ed): string => ed[0]; | |
| export const getSerial = (ed: EnvDisk): string => ed[0]; |
and use it everywhere. Makes the code a lot more readable
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.
| export const getSource = (ed): EnvVarSource => ed[1]; | |
| export const getSource = (ed: EnvDisk): EnvVarSource => ed[1]; |
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.
| export const getSourceKind = (envDisk: (string | object | number)[]): string => { | |
| export const getSourceKind = (envDisk: EnvDisk): 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.
| envDisk1: (string | object | number)[], | |
| envDisk1: EnvDisk, |
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.
| const updateEnvDisks = (newEnvDisks) => { | |
| const updateEnvDisks = (newEnvDisks: NameValuePairs) => { |
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.
| export const setNewSourceDisk = (diskName: string, serial: string, diskBus: string) => { | |
| export const setNewSourceDisk = (diskName: string, serial: string, diskBus: string): V1Disk => { |
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.
no need to specify V1Volume as setNewSourceVolume has correct signature
| const newVolume: V1Volume = setNewSourceVolume( | |
| const newVolume = setNewSourceVolume( |
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.
| let newSourcesDisks = []; | |
| let newSourcesDisks: V1Disk[] = []; |
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.
| let newSourcesVolumes = []; | |
| let newSourcesVolumes: V1Volume[] = []; |
- Add an Environment tab for virtual machines - The user can now add configmaps, secrets and service accounts to VMs as disks but has to mount them manually. (not all images support CloudInit) Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>
5bb0ed6 to
734f8fc
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irosenzw, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
to VMs as disks but has to mount them manually.
(not all images support CloudInit)
Signed-off-by: Ido Rosenzwig irosenzw@redhat.com