Skip to content
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.

Sign up for GitHub

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

KLO-128 : user should be able to select nodespool while creating application and project managed services #118

Merged
merged 3 commits into from
Mar 4, 2024
Merged

KLO-128 : user should be able to select nodespool while creating application and project managed services #118

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efe541f
fix(web-console): add node selector for below resource:
nxtCoder19 Mar 4, 2024
7ad4ba5
add node selector to app and project managed services
nxtCoder19 Mar 4, 2024
1372468
add node selector to app and project managed services
nxtCoder19 Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 44 additions & 1 deletion src/apps/console/routes/_main+/$account+/$project+/$environment+/new-app/app-compute.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import Yup from '~/root/lib/server/helpers/yup';
import { FadeIn, parseValue } from '~/console/page-components/util';
import Select from '~/components/atoms/select';
import ExtendedFilledTab from '~/console/components/extended-filled-tab';
import { parseNodes } from '~/console/server/r-utils/common';
import {parseName, parseNodes} from '~/console/server/r-utils/common';
import useCustomSwr from '~/lib/client/hooks/use-custom-swr';
import { useConsoleApi } from '~/console/server/gql/api-provider';
import { useMapper } from '~/components/utils';
import { registryHost } from '~/lib/configs/base-url.cjs';
import { BottomNavigation } from '~/console/components/commons';
import { plans } from './datas';
import {useOutletContext } from '@remix-run/react';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (llm): The refactored code introduces additional complexity compared to the original version, primarily due to increased dependencies, more intricate state management, and the addition of new UI elements. Here are a few suggestions to streamline the implementation:

  1. Abstract Data Fetching: Consider encapsulating the data fetching logic into custom hooks or services. This not only reduces the complexity within the component but also makes the data fetching logic reusable and easier to maintain.

  2. Simplify State Management: There's an opportunity to simplify state management by consolidating related states or employing a state management library that offers a cleaner way to manage dependencies between states.

  3. Remove Debugging Code: It's generally advisable to remove console.log statements from production code. If logging is necessary, consider implementing a structured logging approach that can be toggled based on the environment.

  4. UI Components Abstraction: For UI patterns that recur, such as form fields with validation, creating abstracted components can reduce duplication and simplify the main component. This encapsulates common logic and styling, making the overall codebase more manageable.

By addressing these points, the code can be made more maintainable and easier to understand, enhancing its long-term scalability.

import {IAppContext} from "~/console/routes/_main+/$account+/$project+/$environment+/app+/$app+/_layout";

const valueRender = ({
label,
Expand Down Expand Up @@ -47,6 +49,9 @@ const AppCompute = () => {
getImageTag,
} = useAppState();
const api = useConsoleApi();
const {cluster} = useOutletContext<IAppContext>()

console.log("cluster", parseName(cluster))

const {
data,
Expand All @@ -56,6 +61,15 @@ const AppCompute = () => {
return api.listRepo({});
});

const {
data: nodepoolData,
isLoading: nodepoolLoading,
error: nodepoolLoadingError,
} = useCustomSwr('/nodepools', async () => {
return api.listNodePools({clusterName: parseName(cluster)})
})

console.log("np", nodepoolData)
const { values, errors, handleChange, isLoading, submit } = useForm({
initialValues: {
imageUrl: app.spec.containers[activeContIndex]?.image || '',
Expand Down Expand Up @@ -97,6 +111,8 @@ const AppCompute = () => {
app.spec.containers[activeContIndex].resourceMemory?.max,
0
),

nodepoolName: app.spec.nodeSelector?.[keyconstants.nodepoolName] || ''
},
validationSchema: Yup.object({
pullSecret: Yup.string(),
Expand All @@ -121,6 +137,10 @@ const AppCompute = () => {
},
spec: {
...s.spec,
nodeSelector: {
...(s.spec.nodeSelector || {}),
[keyconstants.nodepoolName]: val.nodepoolName
},
containers: [
{
...(s.spec.containers?.[0] || {}),
Expand Down Expand Up @@ -166,6 +186,11 @@ const AppCompute = () => {
accName: val.accountName,
}));

const nodepools = useMapper(parseNodes(nodepoolData), (val) => ({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): The addition of nodepool selection in the app compute configuration is a significant feature. To ensure its robustness, unit tests should be added to verify that the nodepool selection is correctly passed to the app's spec. This includes testing the handling of the selected nodepool name and ensuring it's correctly applied.

label: val.metadata?.name || '',
value: val.metadata?.name || '',
}))

const {
data: digestData,
isLoading: digestLoading,
Expand Down Expand Up @@ -196,6 +221,24 @@ const AppCompute = () => {
manipulation and calculations in a system.
</div>
<div className="flex flex-col gap-3xl">

<Select
label="Nodepool Name"
size="lg"
placeholder="Select Nodepool"
value={values.nodepoolName}
creatable
onChange={(val) => {
handleChange('nodepoolName')(dummyEvent(val.value));
}}
options={async () => [...nodepools]}
error={!!errors.repos || !!nodepoolLoadingError}
message={
nodepoolLoadingError ? 'Error fetching nodepools.' : errors.app
}
loading={nodepoolLoading}
/>

<Select
label="Repo Name"
size="lg"
Expand Down
16 changes: 12 additions & 4 deletions src/apps/console/routes/_main+/$account+/$project+/$environment+/new-app/app-review.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
BottomNavigation,
ReviewComponent,
} from '~/console/components/commons';
import {keyconstants} from "~/console/server/r-utils/key-constants";

const AppReview = () => {
const { app, setPage, resetState } = useAppState();
Expand Down Expand Up @@ -114,12 +115,19 @@ const AppReview = () => {
</div>
</div>
</ReviewComponent>
<ReviewComponent
title="Environment"
<ReviewComponent title="Nodepool Details" onEdit={() => {}}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (llm): The addition of the "Nodepool Details" section via a new ReviewComponent is well-integrated into the existing structure of the component. It adheres to the established patterns, maintaining consistency and readability, which is beneficial for future maintenance. The change is relatively isolated, focusing on adding new information without altering the logic of existing sections. This approach minimizes the risk of introducing bugs or complexities. The use of keyconstants for accessing specific data points is in line with how other data is managed within the component, ensuring a coherent development pattern. Overall, this update appears to enhance the component's functionality without significantly increasing its complexity.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): The inclusion of nodepool details in the app review step is a good addition. It would be beneficial to have end-to-end tests that cover the scenario of selecting a nodepool and verifying that this selection is accurately reflected in the review step. This ensures that the user's selection is correctly captured and displayed.

<div className="flex flex-col p-xl gap-md rounded border border-border-default">
<div className="bodyMd-semibold text-text-default">
Nodepool Selector
</div>
<div className="bodySm text-text-soft">{app.spec.nodeSelector[keyconstants.nodepoolName]}</div>
</div>
</ReviewComponent>
<ReviewComponent
title="Environment"
onEdit={() => {
setPage(3);
}}
>
}}>
<div className="flex flex-col gap-xl p-xl rounded border border-border-default">
<div className="flex flex-row items-center gap-lg pb-xl border-b border-border-default">
<div className="flex-1 bodyMd-medium text-text-default">
Expand Down
78 changes: 67 additions & 11 deletions src/apps/console/routes/_main+/$account+/$project+/new-managed-service/_index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import { Switch } from '~/components/atoms/switch';
import { NumberInput, TextInput } from '~/components/atoms/input';
import { handleError } from '~/root/lib/utils/common';
import { titleCase } from '~/components/utils';
import {titleCase, useMapper} from '~/components/utils';
import { flatMapValidations, flatM } from '~/console/utils/commons';
import MultiStepProgress, {
useMultiStepProgress,
Expand All @@ -25,6 +25,10 @@ import {
ReviewComponent,
} from '~/console/components/commons';
import { IProjectContext } from '../_layout';
import {parseName, parseNodes} from "~/console/server/r-utils/common";
import useCustomSwr from "~/lib/client/hooks/use-custom-swr";
import {INodepools} from "~/console/server/gql/queries/nodepool-queries";
import {keyconstants} from "~/console/server/r-utils/key-constants";

const valueRender = ({ label, icon }: { label: string; icon: string }) => {
return (
Expand Down Expand Up @@ -239,6 +243,7 @@ const TemplateView = ({

const FieldView = ({
selectedTemplate,
nodepools,
values,
handleSubmit,
handleChange,
Expand All @@ -249,6 +254,7 @@ const FieldView = ({
values: Record<string, any>;
errors: Record<string, any>;
selectedTemplate: ISelectedTemplate | null;
nodepools: {label: string, value: string}[]
}) => {
const nameRef = useRef<HTMLInputElement>(null);
useEffect(() => {
Expand Down Expand Up @@ -276,6 +282,21 @@ const FieldView = ({
handleChange={handleChange}
nameErrorLabel="isNameError"
/>

<Select
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): The implementation of the 'Select' component for 'Nodepool Name' is well-integrated with the form's state management and validation schema. The use of async options loading and the 'creatable' prop provides a flexible user experience. However, consider handling the scenario where the async loading fails more gracefully, ensuring the user is informed and can retry or proceed with manual input.

label="Nodepool Name"
size="lg"
placeholder="Select Nodepool"
value={values.nodepoolName}
creatable
onChange={(val) => {
handleChange('nodepoolName')(dummyEvent(val.value));
}}
options={async () => [...nodepools]}
error={!!errors.nodepoolName}
message={errors.nodepoolName}
/>

{selectedTemplate?.template.fields?.map((field) => {
const k = field.name;
const x = k.split('.').reduce((acc, curr) => {
Expand Down Expand Up @@ -368,21 +389,32 @@ const ReviewView = ({
</div>
</div>
</ReviewComponent>

<ReviewComponent
title="Service detail"
onEdit={() => {
onEdit(1);
}}
>
<div className="flex flex-col p-xl gap-md rounded border border-border-default">
<div className="bodyMd-semibold text-text-default">
{values?.selectedTemplate?.categoryDisplayName}
title="Service details"
onEdit={() => {
onEdit(1);
}}>
<div className="flex flex-col gap-xl p-xl rounded border border-border-default">
<div className="flex flex-col gap-lg pb-xl border-b border-border-default">
<div className="flex-1 bodyMd-medium text-text-default">
{values?.selectedTemplate?.categoryDisplayName}
</div>
<div className="text-text-soft bodyMd">
{values?.selectedTemplate?.template?.displayName}
</div>
</div>
<div className="bodySm text-text-soft">
{values?.selectedTemplate?.template?.displayName}
<div className="flex flex-col gap-lg">
<div className="flex-1 bodyMd-medium text-text-default">
Node Selector
</div>
<div className="text-text-soft bodyMd">
{values.nodepoolName}
</div>
</div>
</div>
</ReviewComponent>

{renderFieldView()}
{values?.res?.resources && (
<ReviewComponent
Expand Down Expand Up @@ -436,6 +468,17 @@ const ManagedServiceLayout = () => {
const { project, account } = useParams();
const rootUrl = `/${account}/${project}/managed-services`;

const { cluster } = useOutletContext<IProjectContext>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (llm): The addition of new logic for fetching and managing node pools significantly increases the complexity of this component. This includes more state variables, asynchronous data fetching, and additional UI logic for handling the selection of node pools. While these changes are necessary for the new functionality, there are opportunities to simplify the code and improve maintainability.

Consider abstracting the complex logic related to fetching and transforming node pool data into a custom hook. This would encapsulate the asynchronous operations and data processing, making the main component more focused on rendering and interaction logic. For example, a useNodePools hook could handle fetching, transforming, and filtering the node pools, exposing only the final data and loading states to the component.

Simplifying state management by reducing the number of state variables or centralizing their management can also help make the component easier to understand and reduce the potential for bugs. Additionally, consider lazy loading components or data that are not critical for the initial render to improve performance and reduce the initial load complexity.

By addressing these points, the component can be made more readable and easier to maintain, while still fulfilling its required functionality.

console.log("cluster", parseName(cluster))

const {
data: nodepoolData,
isLoading: nodepoolLoading,
error: nodepoolLoadingError,
} = useCustomSwr('/nodepools', async () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): It's great to see the use of useCustomSwr for fetching nodepool data asynchronously. However, I recommend adding unit tests to ensure that the nodepool data is fetched correctly and handled properly in case of errors. This is crucial for the reliability of the feature allowing users to select nodepools.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): Ensure to handle loading and error states gracefully in the UI for a better user experience. This could include displaying loading indicators while the nodepools are being fetched and showing error messages if the fetch fails. Testing these scenarios to ensure they're handled correctly would be beneficial.

return api.listNodePools({clusterName: parseName(cluster)})
})

const { currentStep, jumpStep, nextStep } = useMultiStepProgress({
defaultStep: 1,
totalSteps: 3,
Expand All @@ -449,6 +492,7 @@ const ManagedServiceLayout = () => {
res: {},
selectedTemplate: null,
isNameError: false,
nodepoolName: ''
},
validationSchema: Yup.object().shape({
name: Yup.string().test('required', 'Name is required', (v) => {
Expand Down Expand Up @@ -513,6 +557,9 @@ const ManagedServiceLayout = () => {

spec: {
msvcSpec: {
nodeSelector: {
[keyconstants.nodepoolName]: val.nodepoolName
},
serviceTemplate: {
apiVersion: selectedTemplate.template.apiVersion,
kind: selectedTemplate.template.kind,
Expand Down Expand Up @@ -550,6 +597,14 @@ const ManagedServiceLayout = () => {
},
});

const nodepools = useMapper(parseNodes(nodepoolData), (val) => ({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (llm): I noticed that you're filtering nodepools to only include those with a 'stateful' nodepoolStateType. It would be beneficial to include integration tests that verify the filtering logic works as expected, especially in scenarios with a mix of stateful and other types of nodepools.

label: val.metadata?.name || '',
value: val.metadata?.name || '',
nodepoolStateType: val.spec.nodeLabels[keyconstants.nodepoolStateType]
}))

const statefulNodepools = nodepools.filter((np) => np.nodepoolStateType == 'stateful')

useEffect(() => {
const selectedTemplate =
values.selectedTemplate as unknown as ISelectedTemplate;
Expand Down Expand Up @@ -594,6 +649,7 @@ const ManagedServiceLayout = () => {
errors={errors}
handleChange={handleChange}
handleSubmit={handleSubmit}
nodepools={statefulNodepools}
/>
</MultiStepProgress.Step>
<MultiStepProgress.Step label="Review" step={3}>
Expand Down
3 changes: 2 additions & 1 deletion src/apps/console/server/r-utils/key-constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const keyconstants = {
repoName: 'kloudlite.io/ui-repoName',
imageTag: 'kloudlite.io/ui-imageTag',
repoAccountName: 'kloudlite.io/ui-repoAccountName',
nodepoolName: 'kloudlite.io/nodepool.name',

nodepoolStateType: 'kloudlite.io/ui/pool.app-state-type'
nodepoolStateType: 'kloudlite.io/ui-pool.app-state-type'
};
Loading