Copilot commented on code in PR #12676: URL: https://github.com/apache/cloudstack/pull/12676#discussion_r2839046930
########## ui/src/utils/advisory/index.js: ########## @@ -0,0 +1,76 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +import { getAPI } from '@/api' + +/** + * Generic helper to check if an API has no items (useful for advisory conditions) + * @param {Object} store - Vuex store instance + * @param {string} apiName - Name of the API to call (e.g., 'listNetworks') + * @param {Object} params - Optional parameters to merge with defaults + * @param {Function} filterFunc - Optional function to filter items. If provided, returns true if no items match the filter. + * @param {string} itemsKey - Optional key for items array in response. If not provided, will be deduced from apiName + * @returns {Promise<boolean>} - Returns true if no items exist (advisory should be shown), false otherwise + */ +export async function hasNoItems (store, apiName, params = {}, filterFunc = null, itemsKey = null) { + if (!(apiName in store.getters.apis)) { + return false + } + + // If itemsKey not provided, deduce it from apiName + if (!itemsKey) { + // Remove 'list' prefix: listNetworks -> Networks + let key = apiName.replace(/^list/i, '') + // Convert to lowercase + key = key.toLowerCase() + // Handle plural forms: remove trailing 's' or convert 'ies' to 'y' + if (key.endsWith('ies')) { + key = key.slice(0, -3) + 'y' + } else if (key.endsWith('s')) { + key = key.slice(0, -1) + } + itemsKey = key + } + + const allParams = { + listall: true, + ...params + } + + if (filterFunc == null) { + allParams.page = 1 + allParams.pageSize = 1 + } + + console.debug(`Checking if API ${apiName} has no items with params`, allParams) + + try { + const json = await getAPI(apiName, allParams) + // Auto-derive response key: listNetworks -> listnetworksresponse + const responseKey = `${apiName.toLowerCase()}response` + const items = json?.[responseKey]?.[itemsKey] || [] + if (filterFunc) { + 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 Review 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. ```suggestion 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 ``` ########## ui/src/utils/advisory/index.js: ########## @@ -0,0 +1,76 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +import { getAPI } from '@/api' + +/** + * Generic helper to check if an API has no items (useful for advisory conditions) + * @param {Object} store - Vuex store instance + * @param {string} apiName - Name of the API to call (e.g., 'listNetworks') + * @param {Object} params - Optional parameters to merge with defaults + * @param {Function} filterFunc - Optional function to filter items. If provided, returns true if no items match the filter. + * @param {string} itemsKey - Optional key for items array in response. If not provided, will be deduced from apiName + * @returns {Promise<boolean>} - Returns true if no items exist (advisory should be shown), false otherwise + */ +export async function hasNoItems (store, apiName, params = {}, filterFunc = null, itemsKey = null) { + if (!(apiName in store.getters.apis)) { + return false + } + + // If itemsKey not provided, deduce it from apiName + if (!itemsKey) { + // Remove 'list' prefix: listNetworks -> Networks + let key = apiName.replace(/^list/i, '') + // Convert to lowercase + key = key.toLowerCase() + // Handle plural forms: remove trailing 's' or convert 'ies' to 'y' + if (key.endsWith('ies')) { + key = key.slice(0, -3) + 'y' + } else if (key.endsWith('s')) { + key = key.slice(0, -1) + } + itemsKey = key + } + + const allParams = { + listall: true, + ...params + } + + if (filterFunc == null) { + allParams.page = 1 + allParams.pageSize = 1 + } + + console.debug(`Checking if API ${apiName} has no items with params`, allParams) + + try { + const json = await getAPI(apiName, allParams) + // Auto-derive response key: listNetworks -> listnetworksresponse + const responseKey = `${apiName.toLowerCase()}response` + const items = json?.[responseKey]?.[itemsKey] || [] + if (filterFunc) { + 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 + } + return items.length === 0 + } catch (error) { Review 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. ```suggestion } catch (error) { console.error(`Failed to fetch items for advisory check via API ${apiName}`, error) ``` ########## ui/src/config/section/compute.js: ########## @@ -647,19 +750,8 @@ export default { id: 'cks-version-check', severity: 'warning', message: 'message.advisory.cks.version.check', Review 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. ```suggestion message: 'message.advisory.cks.version.check', docsHelp: 'plugins/cloudstack-kubernetes-service.html', ``` ########## ui/src/config/section/compute.js: ########## @@ -647,19 +750,8 @@ export default { id: 'cks-version-check', severity: 'warning', message: 'message.advisory.cks.version.check', - docsHelp: 'plugins/cloudstack-kubernetes-service.html', - dismissOnConditionFail: true, condition: async (store) => { - const api = 'listKubernetesSupportedVersions' - if (!(api in store.getters.apis)) { - return false - } - try { - const json = await getAPI(api, {}) - const versions = json?.listkubernetessupportedversionsresponse?.kubernetessupportedversion || [] - return versions.length === 0 - } catch (error) {} - return false + return await hasNoItems(store, 'listKubernetesSupportedVersions') Review Comment: The removal of `dismissOnConditionFail: true` from the `cks-min-offering` and `cks-version-check` advisories changes their behavior. Without this property, these advisories will reappear each time the user navigates to the page even after the condition becomes false (i.e., when offerings/versions are added). The property was previously present in the old implementation and controls whether the advisory should be auto-dismissed when the condition is no longer met. This may lead to a degraded user experience where resolved advisories keep appearing. ########## ui/src/config/section/network.js: ########## @@ -397,6 +398,66 @@ export default { tabs: [{ component: shallowRef(defineAsyncComponent(() => import('@/views/compute/InstanceTab.vue'))) }], + advisories: [ + { + id: 'vnfapp-image-check', + severity: 'warning', + message: 'message.advisory.vnf.appliance.template.missing', + condition: async (store) => { + return await hasNoItems(store, + 'listVnfTemplates', + { isvnf: true, templatefilter: 'executable', isready: true }) Review 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. ```suggestion { isvnf: true, templatefilter: 'executable', isready: true }, 'listtemplatesresponse.template') ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
