Skip to content

UI: Fix deploy VM wizard vApp properties#8072

Merged
shwstppr merged 1 commit into
apache:4.18from
shapeblue:fix-ui-deploy-as-is-props-deployapi
Oct 25, 2023
Merged

UI: Fix deploy VM wizard vApp properties#8072
shwstppr merged 1 commit into
apache:4.18from
shapeblue:fix-ui-deploy-as-is-props-deployapi

Conversation

@nvazquez

Copy link
Copy Markdown
Contributor

Description

This PR fixes the vApp properties values sent as part of the deployVirtualMachine API through the UI

Fixes: #7998

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested with vApps containing properties registered as 'Read settings from OVA' templates on Vmware env

How did you try to break this feature and the system with this change?

@codecov

codecov Bot commented Oct 11, 2023

Copy link
Copy Markdown

Codecov Report

Merging #8072 (def62d4) into 4.18 (864a195) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               4.18    #8072   +/-   ##
=========================================
  Coverage     13.06%   13.06%           
  Complexity     9108     9108           
=========================================
  Files          2720     2720           
  Lines        257537   257537           
  Branches      40156    40156           
=========================================
  Hits          33658    33658           
  Misses       219649   219649           
  Partials       4230     4230           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@DaanHoogland DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

@DaanHoogland

Copy link
Copy Markdown
Contributor

tested ok with a vstream template. in the ui:
image

call being made:

http://localhost:5050/client/api/?zoneid=b97af5af-6dc9-408e-8e5a-d3918591ab95&dynamicscalingenabled=true&iothreadsenabled=false&templateid=d38fd9a5-f30e-42d1-b0a8-740d14aa1f4f&startvm=true&serviceofferingid=5e380a1c-708b-4f48-8aad-c93b8943f387&affinitygroupids=&nicnetworklist[0].nic=12&nicnetworklist[0].network=4430bd0a-978a-43ad-8184-73a680ef6ffc&nicnetworklist[1].nic=13&nicnetworklist[1].network=4430bd0a-978a-43ad-8184-73a680ef6ffc&keypairs=&name=v1&displayname=v1&properties[0].key=A_IPADDR&properties[0].value=10.10.10.1&properties[1].key=B_NETMASK&properties[1].value=255.255.255.0&properties[2].key=C_DEFGATEWAY&properties[2].value=10.10.10.11&properties[3].key=H_HOSTNAME&properties[3].value=vstream&properties[4].key=I_DOMAINNAME&properties[4].value=bla&properties[5].key=E_DNS1&properties[5].value=10.10.10.11&properties[6].key=F_DNS2&properties[6].value=10.10.10.22&properties[7].key=F_NTP1&properties[7].value=10.10.10.11&properties[8].key=G_NTP2&properties[8].value=10.10.10.22&properties[9].key=A_NGENIUS_SERVER_IPV4&properties[9].value=10.10.10.11&properties[10].key=dataDisk&properties[10].value=20&command=deployVirtualMachine&response=json

@shwstppr shwstppr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@DaanHoogland

Copy link
Copy Markdown
Contributor

@nvazquez , do you need to add to this or can we merge?

@shwstppr shwstppr added this to the 4.18.2.0 milestone Oct 11, 2023
@nvazquez

Copy link
Copy Markdown
Contributor Author

@DaanHoogland I think this is ready but we could double check other vApps sections as well for verification

@borisstoyanov borisstoyanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just ui changes no need for Marvin tests

@harikrishna-patnala harikrishna-patnala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested these changes.

@DaanHoogland
DaanHoogland marked this pull request as ready for review October 18, 2023 10:21
@shwstppr

Copy link
Copy Markdown
Contributor

@blueorangutan ui

@blueorangutan

Copy link
Copy Markdown

@shwstppr a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@shwstppr
shwstppr requested a review from kiranchavala October 19, 2023 11:49
@blueorangutan

Copy link
Copy Markdown

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/8072 (QA-JID-208)

@kiranchavala

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@kiranchavala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7470

@kiranchavala kiranchavala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nvazquez If there are no networks available, the VM deployment form does allow to create a new network

An existing isolated network should be present before launching a vm from a vapp

vapp

vapp2

vapp3

@kiranchavala kiranchavala self-assigned this Oct 23, 2023
@shwstppr

Copy link
Copy Markdown
Contributor

Option to add a new network can be a separate improvement as it has been the same behaviour even before this PR @kiranchavala cc @nvazquez

@kiranchavala kiranchavala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested the changes manually

Before fix

http://10.0.32.243:8080/client/api/?zoneid=6e980031-2cee-4f44-8dcb-88882d635a9a&dynamicscalingenabled=false&iothreadsenabled=false&templateid=422d4275-36d0-4c7f-8130-7bdc76db3042&startvm=true&serviceofferingid=b3bd601b-8103-4fbd-a3b8-a3f22eb60c5d&affinitygroupids=&nicnetworklist[0].nic=12&nicnetworklist[0].network=36e7ca23-5713-4bbe-aff9-e988c31ce604&nicnetworklist[1].nic=13&nicnetworklist[1].network=43bc19cf-6342-4c22-98dd-15c746105680&keypairs=&command=deployVirtualMachine&response=json

After fix

http://10.0.34.107:8080/client/api/?zoneid=b64006da-03e5-40ac-9381-2bae36357555&dynamicscalingenabled=false&iothreadsenabled=false&templateid=8bdbc792-91ba-49fc-8fd9-2234c610c286&startvm=true&serviceofferingid=3f1f2e13-7946-4e39-a30c-9c754630b789&affinitygroupids=&nicnetworklist[0].nic=12&nicnetworklist[0].network=59261057-2eee-47e5-a3a5-0bcd32ca9c87&nicnetworklist[1].nic=13&nicnetworklist[1].network=59261057-2eee-47e5-a3a5-0bcd32ca9c87&keypairs=&properties[0].key=A_IPADDR&properties[0].value=10.10.10.11&properties[1].key=B_NETMASK&properties[1].value=255.255.255.0&properties[2].key=C_DEFGATEWAY&properties[2].value=10.10.10.1&properties[3].key=H_HOSTNAME&properties[3].value=vstream&properties[4].key=I_DOMAINNAME&properties[4].value=&properties[5].key=E_DNS1&properties[5].value=8.8.8.8&properties[6].key=F_DNS2&properties[6].value=8.8.4.4&properties[7].key=F_NTP1&properties[7].value=0.0.0.0&properties[8].key=G_NTP2&properties[8].value=0.0.0.0&properties[9].key=A_NGENIUS_SERVER_IPV4&properties[9].value=0.0.0.0&properties[10].key=dataDisk&properties[10].value=100&command=deployVirtualMachine&response=json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[UI] Vmware deploy-as-is instances are not receiving the properties values

7 participants