-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixes for Import VM Tasks listing #11841
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
Fixes for Import VM Tasks listing #11841
Conversation
@blueorangutan package |
@nvazquez 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11841 +/- ##
=========================================
- Coverage 3.58% 3.58% -0.01%
=========================================
Files 445 445
Lines 37494 37532 +38
Branches 6889 6901 +12
=========================================
Hits 1346 1346
- Misses 35984 36022 +38
Partials 164 164
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15458 |
@blueorangutan package |
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.
code lgtm. Left a few small comments
|
||
@Parameter(name = ApiConstants.SHOW_COMPLETED, type = CommandType.BOOLEAN, description = "Whether to list completed tasks.") | ||
private boolean showCompleted = false; | ||
@Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "List tasks by status, valid options are: Running, Completed, Failed") |
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.
should we add a since field?
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.
Thanks @Pearl1594, the since parameter is added on the API class entirely and it will use that one as all the parameters have been added on the same version (4.22)
private String displayName; | ||
|
||
@SerializedName(ApiConstants.STATE) | ||
@Param(description = "the state of the importing VM task") |
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.
since?
@blueorangutan package |
@nvazquez 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15468 |
@blueorangutan test |
@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
@blueorangutan package |
@nvazquez 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. |
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15469 |
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.
LGTM based on actual testing, UI improvements are nice, good job
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.
One comment otherwise code looks good
|
||
@Parameter(name = ApiConstants.SHOW_COMPLETED, type = CommandType.BOOLEAN, description = "Whether to list completed tasks.") | ||
private boolean showCompleted = false; | ||
@Parameter(name = ApiConstants.TASKS_FILTER, type = CommandType.STRING, description = "Filter tasks by status, valid options are: All, Running, Completed, Failed") |
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.
Would it be better to name it state or status?
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.
Thanks @shwstppr, fixed this description to match with state
[SF] Trillian test result (tid-14690)
|
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.
LGTM
* Fix import VM tasks pagination * Fix UI for pagination and proper listing * Fixes and improvements * Polish UI * Restore config.json * Fix state on parameter description
Description
This PR fixes UI pagination and small issues on the Import VM Tasks listing
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?