Copilot commented on code in PR #13647:
URL: https://github.com/apache/cloudstack/pull/13647#discussion_r3622499820
##########
ui/src/components/view/ActionButton.vue:
##########
@@ -177,6 +177,15 @@ export default {
methods: {
execAction (action) {
action.resource = this.resource
+ const record = this.resource || this.item
+ if (action?.api === 'disableOutOfBandManagementForHost' &&
(record?.hostha?.haenable === true || record?.hastate === 'Enabled')) {
Review Comment:
`execAction` reads `this.item`, but `item` is not defined as a
prop/data/computed on this component. In Vue this can trigger runtime warnings
and the fallback is ineffective (since `resource` defaults to an object). Use
`this.resource` (or explicitly add an `item` prop if that’s intended).
##########
ui/src/config/section/infra/hosts.js:
##########
@@ -274,7 +279,13 @@ export default {
docHelp: 'adminguide/reliability.html#ha-for-hosts',
dataView: true,
show: (record) => {
- return record.hypervisor !== 'External' && !(record?.hostha?.haenable
=== true)
+ if (record.hypervisor === 'External' || record?.hostha?.haenable ===
true) {
+ return false
+ }
+ if (record.hypervisor === 'KVM') {
+ return Boolean(record?.outofbandmanagement?.enabled)
+ }
Review Comment:
Same as above: use a strict `=== true` check for
`outofbandmanagement.enabled` to keep behavior consistent with the rest of the
host actions and avoid truthiness surprises.
##########
ui/src/config/section/infra/hosts.js:
##########
@@ -255,7 +255,12 @@ export default {
message: 'label.ha.configure',
docHelp: 'adminguide/reliability.html#ha-for-hosts',
dataView: true,
- show: (record) => { return ['KVM',
'Simulator'].includes(record.hypervisor) },
+ show: (record) => {
+ if (record.hypervisor === 'KVM') {
+ return Boolean(record?.outofbandmanagement?.enabled)
+ }
+ return record.hypervisor === 'Simulator'
+ },
Review Comment:
`outofbandmanagement.enabled` is treated as a strict boolean elsewhere in
this file (e.g., `=== true`). Using `Boolean(...)` here changes semantics for
unexpected values (e.g. string "false" becomes truthy) and is inconsistent with
the existing checks.
##########
ui/src/components/view/ListView.vue:
##########
@@ -693,16 +692,21 @@
<template v-if="text">
<template v-if="!text.startsWith('PrjAcct-')">
<router-link
- v-if="$route.path.startsWith('/quotasummary') &&
$router.resolve(`${$route.path}/${record.accountid}`).matched[0].redirect !==
'/exception/404'"
+ v-if="$route.path.startsWith('/quotasummary')"
:to="{ path: `${$route.path}/${record.accountid}` }">{{ text
}}</router-link>
Review Comment:
The quota summary account link no longer guards against missing/invalid
routes (the previous `$router.resolve(...).matched[0].redirect !==
'/exception/404'` check). This can reintroduce navigation to the 404 route in
setups where `/quotasummary/:accountid` isn’t registered.
##########
ui/src/components/view/ListView.vue:
##########
@@ -655,8 +655,7 @@
<template v-if="column.key === 'usageType'">
{{ usageTypeMap[record.usagetype] }}
</template>
-
- <template v-if="column.key === 'clustername'">
+<template v-if="column.key === 'clustername'">
Review Comment:
This `<template>` line is mis-indented compared to the surrounding template
blocks, which makes the section harder to read and can fail format/lint checks
if enforced.
##########
ui/src/components/view/ListView.vue:
##########
@@ -693,16 +692,21 @@
<template v-if="text">
<template v-if="!text.startsWith('PrjAcct-')">
<router-link
- v-if="$route.path.startsWith('/quotasummary') &&
$router.resolve(`${$route.path}/${record.accountid}`).matched[0].redirect !==
'/exception/404'"
+ v-if="$route.path.startsWith('/quotasummary')"
:to="{ path: `${$route.path}/${record.accountid}` }">{{ text
}}</router-link>
- <span v-else>{{ text }}</span>
- </template>
- <template v-else>
+ <router-link v-else-if="record.accountid" :to="{ path: '/account/'
+ record.accountid }">{{ text }}</router-link>
<router-link
- v-if="$route.path.startsWith('/quotasummary') &&
$router.resolve(`${$route.path}/${record.accountid}`).matched[0].redirect !==
'/exception/404'"
- :to="{ path: `${$route.path}/${record.accountid}` }">{{
(record.projectname || record.account).concat('
(').concat($t('label.project')).concat(')') }}</router-link>
+ v-else-if="$store.getters.userInfo.roletype !== 'User'"
+ :to="{ path: '/account', query: { name: record.account,
domainid: record.domainid, dataView: true } }">
+ {{ text }}
+ </router-link>
<span v-else>{{ text }}</span>
Review Comment:
This PR is described as an HA/OOBM validation fix for KVM hosts, but this
hunk substantially changes account-linking behavior in the shared `ListView`
(new routes, removed 404 guard, different link targets). If this is
intentional, it likely deserves its own PR/description since it can affect many
list views beyond hosts.
--
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]
