fix(website): fix issue with create website with local php-fpm failed#8379
fix(website): fix issue with create website with local php-fpm failed#8379f2c-ci-robot[bot] merged 1 commit intodev-v2from
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| /> | ||
| </template> | ||
| <template v-if="!openNginxConfig" #main> | ||
| <ComplexTable |
There was a problem hiding this comment.
There are minor changes to your code. Here is an updated list of observations:
-
Removed Duplicate
divElement: The line<div class="!ml-2.5">...</div>seems redundant due to the preceding elements. -
Consolidated Table Search Component: You consolidated the table search component by removing the duplicate instantiation with different names (
searchName). -
Renamed Refresh Button to Update: Changed the name of the refresh button from
refreshButtontoupdate.
Here's a concise version of the updated code block:
@@ -51,18 +51,17 @@
</div>
</el-select>
<TableSearch @search="search()" v-model:searchName="req.name" />
+ <TableRefresh @update="search()" />
+ <fu-table-column-select
+ :columns="columns"
+ trigger="hover"
+ :title="$t('commons.table.selectColumn')"
+ popper-class="popper-class"
+ :only-icon="true"
+ />
</template>
<template v-if="!openNginxConfig" #main>
<ComplexTableThis should eliminate unnecessary redundancy and improve clarity in the component structure.
| return row.status === 'Recreating' || row.status === 'Building' || row.resource == 'local'; | ||
| case 'edit': | ||
| return row.status === 'Recreating' || row.status === 'Building'; | ||
| case 'extension': |
There was a problem hiding this comment.
There is an issue with this code in terms of logic. Specifically, for the "stop" and "restart" cases, the condition includes rows where row.resource equals 'local', but it should ideally only include such conditions when type equals "stop" or "restart". This discrepancy could cause unexpected behavior.
Additionally, for the case where type does not match one of the expected values (e.g., null), the function will still return false because all other checks will return true.
Here's a more precise implementation:
import { Runtime } from '@/api/interface/runtime';
export function disabledButton(row: Runtime.Runtime, type: string): boolean {
switch (type) {
case 'stop':
return (
row.status === 'Recreating' ||
row.status === 'Stopped' ||
row.status === 'Building'
);
case 'start':
return (
row.status !== 'Started' &&
row.status !== 'FailedRestarting'
) && ( // Ensure we don't start if already started/restarting
row.status !== 'Recreating' ||
row.status !== 'Stopped' ||
row.status !== 'Building'
)
) ? false : true; // True if starting is allowed by resource status
case 'restart': // Handle restart slightly differently to allow local resources
return row.status === 'ReCreating' || row.status === 'Building' || row.resource === 'local';
case 'edit':
return row.status === 'Recreating' || row.status === 'Building';
case 'extension':
return row.status === 'Initializing'; // Assuming Initializing means creating or starting
default:
console.warn(`Unsupported operation type ${type}. Returning true.`);
return true;
}
}Changes Made:
- Corrected conditions for "stop" and "restart".
- Added additional conditions for handling non-supported types.
- Ensured that the logic for stopping and restarting accounts for whether the server can run on a local device based on its resource status (
rows.resource = 'local').
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
There is an issue with the line if (runtimeResource.value == 'local') where it should be ===. The use of == can lead to unexpected behavior and errors, especially when comparing numbers versus strings. Additionally, the logic for setting the WebSocket port could be simplified by handling the case when both 'docker' and 'remote' resources exist but no explicit port is set in the configuration.
Here's a corrected version:
const changeRuntime = (runID: number) => {
runtimes.value.forEach((item) => {
if (item.id === runID) {
runtimeResource.value = item.resource;
runtimePorts.value = item.port ? item.port.split(',').map((port: string) => parseInt(port.trim(), 10)) : [];
website.value.port = runtime ports.length > 0 ? runtimePorts[0] : null;
// Optimization suggestion: Handle cases when multiple resource types are present
if ((item.resource === 'docker' && !website.value.port) ||
(item.resource === 'remote' && !website.value.port)) {
console.warn(`No specified port found for resource type '${item.resource}'. Using default port.`);
website.value.port = 9000;
}
break; // Once a matching run ID is found, exit the loop
}
});
};In this version:
- The comparison operator used for checking
runtimeResource.valuehas been changed to===. - A check for whether
Website.value.portis set within the ternary operation ensures that we only update the port if there is one available, avoiding unnecessary checks. - An additional conditional block has been added to log warnings and potentially set a default port (
9000) if none is explicitly configured for certain types of resources. This simplifies the code by reducing redundant conditions.
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wanghe-fit2cloud The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
No description provided.