Skip to content
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

ui: fix labels when migrating instances from vmware #8490

Merged
merged 3 commits into from Jan 11, 2024

Conversation

shwstppr
Copy link
Contributor

@shwstppr shwstppr commented Jan 10, 2024

Description

Fixes #8474
Renames labels when importing from VMware

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):

ui-instances-label.mp4

How Has This Been Tested?

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

Fixes apache#8474

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4d42b8d) 4.39% compared to head (a47955b) 30.78%.
Report is 4 commits behind head on main.

Files Patch % Lines
ui/src/views/tools/ManageInstances.vue 0.00% 1 Missing ⚠️
ui/src/views/tools/SelectVmwareVcenter.vue 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              main    #8490       +/-   ##
============================================
+ Coverage     4.39%   30.78%   +26.39%     
- Complexity       0    33978    +33978     
============================================
  Files          361     5341     +4980     
  Lines        28622   374942   +346320     
  Branches      4993    54538    +49545     
============================================
+ Hits          1258   115440   +114182     
- Misses       27225   244240   +217015     
- Partials       139    15262    +15123     
Flag Coverage Δ
simulator-marvin-tests 24.69% <ø> (?)
uitests 4.39% <0.00%> (-0.01%) ⬇️
unit-tests 16.47% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

Code LGTM

@nvazquez
Copy link
Contributor

@blueorangutan ui

@blueorangutan
Copy link

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

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
@shwstppr shwstppr marked this pull request as ready for review January 10, 2024 12:19
@shwstppr
Copy link
Contributor Author

@blueorangutan package

@shwstppr shwstppr added this to the 4.19.0.0 milestone Jan 10, 2024
@blueorangutan
Copy link

@shwstppr 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

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

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

Comment on lines +326 to +327
{{ (isMigrateFromVmware && vmwareVcenterType === 'existing') ? $t('label.instances') : $t('label.unmanaged.instances') }}
<a-tooltip :title="(isMigrateFromVmware && vmwareVcenterType === 'existing') ? $t('message.instances.migrate.vmware') : $t('message.instances.unmanaged')">
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't isMigrateFromVmware be enough condition? i.e.

Suggested change
{{ (isMigrateFromVmware && vmwareVcenterType === 'existing') ? $t('label.instances') : $t('label.unmanaged.instances') }}
<a-tooltip :title="(isMigrateFromVmware && vmwareVcenterType === 'existing') ? $t('message.instances.migrate.vmware') : $t('message.instances.unmanaged')">
{{ isMigrateFromVmware ? $t('label.instances') : $t('label.unmanaged.instances') }}
<a-tooltip :title="isMigrateFromVmware ? $t('message.instances.migrate.vmware') : $t('message.instances.unmanaged')">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DaanHoogland current change allows showing Instances label only when the vCenter is an existing one. External vCenter will always considered to have unmanaged instances

Copy link
Contributor

Choose a reason for hiding this comment

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

yes but the label doesn't have to change for that, that's my point.

@blueorangutan
Copy link

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

Copy link
Collaborator

@rajujith rajujith left a comment

Choose a reason for hiding this comment

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

LGTM.
Screenshot 2024-01-11 at 10 26 49 AM

@shwstppr shwstppr merged commit c43b7c0 into apache:main Jan 11, 2024
25 of 26 checks passed
@shwstppr shwstppr deleted the ui-test-migratevmwtokvm branch January 11, 2024 06:29
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jan 12, 2024
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.

Managed VMware instances are listed as unmanaged for Import instances from VMware into a KVM cluster
5 participants