Skip to content

ui: fix filtering readonly details while VM update#5887

Merged
sureshanaparti merged 3 commits into
apache:4.16from
shapeblue:fix-ui-updatevmdetails
Jan 31, 2022
Merged

ui: fix filtering readonly details while VM update#5887
sureshanaparti merged 3 commits into
apache:4.16from
shapeblue:fix-ui-updatevmdetails

Conversation

@shwstppr

@shwstppr shwstppr commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

Description

Fixes #5724

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

Before fix (filtering of readonly details not working properly and updateVirtualMachine is called with rootDiskController setting)
Steps to reproduce:

  • On a VMware env, as a regular user deploy a VM, stop it after deployment. Template shouldn't have nicAdapter specified
  • Try to update VM with nicAdapter from UI by adding the new setting.
  • Observe updateVirtualMachine API is called with rootDiskController in the details
    Screenshot from 2022-01-24 14-05-44

After fix:
Screenshot from 2022-01-24 14-06-35

How Has This Been Tested?

Fixes apache#5724

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

Copy link
Copy Markdown
Contributor Author

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

@blueorangutan

Copy link
Copy Markdown

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/5887 (SL-JID-1055)

@sureshanaparti sureshanaparti 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.

Verified fix. LGTM

@DaanHoogland

Copy link
Copy Markdown
Contributor

code looks good, but can you give a short brief as to how and why this solves the issue? It looks to be functionally the same, the filter only starting on the other end of the list.

@shwstppr

Copy link
Copy Markdown
Contributor Author

@DaanHoogland I've changed loop to iterate from end of the list. As it was removing elements from the list in the loop, original code was having issues while removing multiple items.

@utchoang utchoang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

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

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

Copy link
Copy Markdown
Contributor Author

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

@sureshanaparti

Copy link
Copy Markdown
Contributor

@shwstppr when i add a read-only detail, it is ignored. please check.

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

Copy link
Copy Markdown
Contributor Author

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

@blueorangutan

Copy link
Copy Markdown

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/5887 (SL-JID-1074)

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

@sureshanaparti

Copy link
Copy Markdown
Contributor

Verified latest changes. LGTM.

@sureshanaparti
sureshanaparti merged commit 5f07e4d into apache:4.16 Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Regular domain admin account cannot change nicAdapter “Request Failed (431)”

7 participants