ui: refactor advisories enabled usage and more#12676
ui: refactor advisories enabled usage and more#12676shwstppr wants to merge 3 commits intoapache:mainfrom
Conversation
Considers advisoriesDisabled as false if no value is found in the config or if set to false. Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12676 +/- ##
=========================================
Coverage 17.92% 17.92%
- Complexity 16154 16158 +4
=========================================
Files 5939 5940 +1
Lines 533181 533240 +59
Branches 65237 65237
=========================================
+ Hits 95585 95597 +12
- Misses 426856 426904 +48
+ Partials 10740 10739 -1
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:
|
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
shwstppr
changed the title
|
@shwstppr a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
Pull request overview
This PR refactors advisory system usage in the UI by introducing a centralized hasNoItems utility function and adding new advisories for VNF appliances and instances. The PR aims to improve code maintainability by reducing duplication and providing a consistent approach to checking resource availability for advisory conditions.
Changes:
- Added
hasNoItemsutility function to centralize API-based advisory condition checking - Refactored existing Kubernetes cluster advisories to use the new utility function
- Added new advisories for instance and VNF appliance deployment prerequisites (templates, ISOs, compute offerings, networks)
- Updated router configuration to handle
advisoriesDisabledconfig with proper null/undefined handling - Added new localization messages for the new advisories
- Added
advisoriesDisabledconfig option toconfig.json
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/utils/advisory/index.js | New utility file with hasNoItems helper function for checking API resource availability |
| ui/src/config/section/network.js | Added advisories for VNF appliance deployment (template and compute offering checks) |
| ui/src/config/section/compute.js | Added advisories for instance deployment (image, compute offering, network checks) and refactored CKS advisories |
| ui/src/config/router.js | Improved handling of advisoriesDisabled config with nullish coalescing operator |
| ui/public/locales/en.json | Added localization messages for new advisory warnings |
| ui/public/config.json | Added advisoriesDisabled: false configuration option |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const a = !items.some(filterFunc) | ||
| console.debug(`API ${apiName} has ${items.length} items, after filter has items: ${items.filter(filterFunc)[0]}, returning ${a}`) | ||
| const it = items.filter(filterFunc) | ||
| console.debug(`Filtered items:`, it) | ||
| return a |
There was a problem hiding this comment.
The debug logging code contains redundant logic. Lines 66-67 compute the result and log filtered items, then lines 68-69 recompute the same filtered items for logging. The variable a is computed but could be simplified, and items.filter(filterFunc)[0] is used in logging which may be undefined if no items match. Consider simplifying this to compute the filtered items once and improve the debug output clarity.
| const a = !items.some(filterFunc) | |
| console.debug(`API ${apiName} has ${items.length} items, after filter has items: ${items.filter(filterFunc)[0]}, returning ${a}`) | |
| const it = items.filter(filterFunc) | |
| console.debug(`Filtered items:`, it) | |
| return a | |
| const filteredItems = items.filter(filterFunc) | |
| const hasNoMatchingItems = filteredItems.length === 0 | |
| console.debug(`API ${apiName} has ${items.length} items, filtered items count: ${filteredItems.length}, returning ${hasNoMatchingItems}`) | |
| console.debug('Filtered items:', filteredItems) | |
| return hasNoMatchingItems |
| @@ -647,19 +750,8 @@ export default { | |||
| id: 'cks-version-check', | |||
| severity: 'warning', | |||
| message: 'message.advisory.cks.version.check', | |||
There was a problem hiding this comment.
The removal of docsHelp property from these advisories may be intentional refactoring, but it removes potentially useful documentation links for users. The original advisories had docsHelp: 'plugins/cloudstack-kubernetes-service.html' which helped users understand Kubernetes service setup. If this is intentional, ensure that documentation is accessible through other means.
| message: 'message.advisory.cks.version.check', | |
| message: 'message.advisory.cks.version.check', | |
| docsHelp: 'plugins/cloudstack-kubernetes-service.html', |
| condition: async (store) => { | ||
| return await hasNoItems(store, | ||
| 'listVnfTemplates', | ||
| { isvnf: true, templatefilter: 'executable', isready: true }) |
There was a problem hiding this comment.
The listVnfTemplates API returns a response with the key listtemplatesresponse.template, not listvnftemplatesresponse.vnftemplate. The current itemsKey deduction logic will incorrectly derive vnftemplate and responseKey as listvnftemplatesresponse, which will not match the actual API response structure. This will cause the advisory condition to fail silently and return false. Consider either passing an explicit itemsKey parameter for this API or handling special API name mappings where the response key doesn't match the API name pattern.
| { isvnf: true, templatefilter: 'executable', isready: true }) | |
| { isvnf: true, templatefilter: 'executable', isready: true }, | |
| 'listtemplatesresponse.template') |
| return a | ||
| } | ||
| return items.length === 0 | ||
| } catch (error) { |
There was a problem hiding this comment.
The error handling silently returns false when an API call fails, which could hide legitimate errors and cause advisories not to appear when they should. Consider logging the error or providing more specific error handling, especially since API failures could indicate permission issues or connectivity problems that users should be aware of.
| } catch (error) { | |
| } catch (error) { | |
| console.error(`Failed to fetch items for advisory check via API ${apiName}`, error) |
|
UI build: ✔️ |
3 participants
Description
Considers advisoriesDisabled as false if no value is found in the config or if set to false.
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?