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

fix(web/nodepool): remove stateful type as extra field #112

Merged
merged 1 commit into from
Mar 4, 2024
Merged

fix(web/nodepool): remove stateful type as extra field #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions gql-queries-generator/doc/queries.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,6 @@ query consoleGetNodePool($clusterName: String!, $poolName: String!) {
}
creationTime
displayName
stateful
kind
lastUpdatedBy {
userEmail
Expand Down Expand Up @@ -614,6 +613,7 @@ query consoleGetNodePool($clusterName: String!, $poolName: String!) {
cloudProvider
maxCount
minCount
nodeLabels
}
status {
checks
Expand Down Expand Up @@ -664,7 +664,6 @@ query consoleListNodePools($clusterName: String!, $search: SearchNodepool, $pagi
}
creationTime
displayName
stateful
lastUpdatedBy {
userEmail
userId
Expand Down Expand Up @@ -707,6 +706,7 @@ query consoleListNodePools($clusterName: String!, $search: SearchNodepool, $pagi
cloudProvider
maxCount
minCount
nodeLabels
}
status {
checks
Expand Down
12 changes: 9 additions & 3 deletions src/apps/console/routes/_main+/$account+/infra+/$cluster+/nodepools/handle-nodepool.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { Switch } from '~/components/atoms/switch';
import { NameIdView } from '~/console/components/name-id-view';
import { findNodePlan, nodePlans, provisionTypes } from './nodepool-utils';
import { IClusterContext } from '../_layout';
import {keyconstants} from "~/console/server/r-utils/key-constants";

type IDialog = IDialogBase<ExtractNodeType<INodepools>>;

Expand Down Expand Up @@ -53,7 +54,7 @@ const Root = (props: IDialog) => {
taints: [],
autoScale: props.data.spec.minCount !== props.data.spec.maxCount,
isNameError: false,
stateful: props.data.stateful || false
stateful: props.data.spec.nodeLabels[keyconstants.nodepoolStateType] || false
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 the transition from a direct boolean flag to using a label to indicate statefulness. It's crucial to ensure that all possible values of nodeLabels[keyconstants.nodepoolStateType] are covered in tests, including cases where it might be undefined or have unexpected values.

}
: {
nvidiaGpuEnabled: false,
Expand Down Expand Up @@ -155,9 +156,11 @@ const Root = (props: IDialog) => {
maxCount: Number.parseInt(val.maximum, 10),
minCount: Number.parseInt(val.minimum, 10),
cloudProvider: 'aws',
nodeLabels: {
Copy link

Choose a reason for hiding this comment

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

suggestion (llm): Given the addition of nodeLabels with stateful or stateless values, it's important to add tests verifying that the correct label is applied based on the val.stateful condition. This ensures the logic correctly reflects the intended statefulness of the nodepool.

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 essential to test the merging behavior of nodeLabels to ensure that existing labels are preserved and the stateful/stateless label is correctly added or updated. This is critical for maintaining the integrity of nodepool configurations.

[keyconstants.nodepoolStateType]: val.stateful ? "stateful" : "stateless"
},
...getNodeConf(),
},
stateful: val.stateful || false
},
});
if (e) {
Expand All @@ -173,11 +176,14 @@ const Root = (props: IDialog) => {
},
spec: {
...props.data.spec,
nodeLabels: {
...(props.data.spec.nodeLabels || {}),
[keyconstants.nodepoolStateType]: val.stateful ? "stateful" : "stateless"
},
maxCount: Number.parseInt(val.maximum, 10),
minCount: Number.parseInt(val.minimum, 10),
...getNodeConf(),
},
stateful: val.stateful || false
},
});
if (e) {
Expand Down
4 changes: 2 additions & 2 deletions src/apps/console/server/gql/queries/nodepool-queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ export const nodepoolQueries = (executor: IExecutor) => ({
}
creationTime
displayName
stateful
kind
lastUpdatedBy {
userEmail
Expand Down Expand Up @@ -79,6 +78,7 @@ export const nodepoolQueries = (executor: IExecutor) => ({
cloudProvider
maxCount
minCount
nodeLabels
Copy link

Choose a reason for hiding this comment

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

suggestion (llm): With the introduction of nodeLabels in the GraphQL queries, it's important to verify through tests that these labels are correctly fetched and processed. This includes testing for the presence of the stateful/stateless label and its correct interpretation.

}
status {
checks
Expand Down Expand Up @@ -158,7 +158,6 @@ export const nodepoolQueries = (executor: IExecutor) => ({
}
creationTime
displayName
stateful
lastUpdatedBy {
userEmail
userId
Expand Down Expand Up @@ -201,6 +200,7 @@ export const nodepoolQueries = (executor: IExecutor) => ({
cloudProvider
maxCount
minCount
nodeLabels
}
status {
checks
Expand Down
4 changes: 3 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 @@ -10,5 +10,7 @@ export const keyconstants = {
nodeType: 'kloudlite.io/ui-node-type',
repoName: 'kloudlite.io/ui-repoName',
imageTag: 'kloudlite.io/ui-imageTag',
repoAccountName: 'kloudlite.io/ui-repoAccountName'
repoAccountName: 'kloudlite.io/ui-repoAccountName',

nodepoolStateType: 'kloudlite.io/ui/pool.app-state-type'
};
3 changes: 0 additions & 3 deletions src/generated/gql/sdl.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3020,7 +3020,6 @@ type NodePool {
metadata: Metadata
recordVersion: Int!
spec: Github__com___kloudlite___operator___apis___clusters___v1__NodePoolSpec!
stateful: Boolean!
status: Github__com___kloudlite___operator___pkg___operator__Status
syncStatus: Github__com___kloudlite___api___pkg___types__SyncStatus!
updateTime: Date!
Expand All @@ -3037,7 +3036,6 @@ input NodePoolIn {
kind: String
metadata: MetadataIn
spec: Github__com___kloudlite___operator___apis___clusters___v1__NodePoolSpecIn!
stateful: Boolean!
}

type NodePoolPaginatedRecords {
Expand Down Expand Up @@ -3477,7 +3475,6 @@ input SearchNamespaces {
}

input SearchNodepool {
isStateful: MatchFilterIn
text: MatchFilterIn
}

Expand Down
6 changes: 2 additions & 4 deletions src/generated/gql/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,6 @@ export type SearchNamespaces = {
};

export type SearchNodepool = {
isStateful?: InputMaybe<MatchFilterIn>;
text?: InputMaybe<MatchFilterIn>;
};

Expand Down Expand Up @@ -962,7 +961,6 @@ export type NodePoolIn = {
kind?: InputMaybe<Scalars['String']['input']>;
metadata?: InputMaybe<MetadataIn>;
spec: Github__Com___Kloudlite___Operator___Apis___Clusters___V1__NodePoolSpecIn;
stateful: Scalars['Boolean']['input'];
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 that the removal of the stateful field from the GraphQL schema does not impact existing queries and mutations negatively. It would be beneficial to add tests that confirm the new nodeLabels structure is correctly handled in all relevant GraphQL operations.

};

export type Github__Com___Kloudlite___Operator___Apis___Clusters___V1__NodePoolSpecIn =
Expand Down Expand Up @@ -2016,7 +2014,6 @@ export type ConsoleGetNodePoolQuery = {
clusterName: string;
creationTime: any;
displayName: string;
stateful: boolean;
kind?: string;
markedForDeletion?: boolean;
updateTime: any;
Expand All @@ -2035,6 +2032,7 @@ export type ConsoleGetNodePoolQuery = {
cloudProvider: Github__Com___Kloudlite___Operator___Apis___Common____Types__CloudProvider;
maxCount: number;
minCount: number;
nodeLabels?: any;
aws?: {
availabilityZone: string;
iamInstanceProfileRole?: string;
Expand Down Expand Up @@ -2104,7 +2102,6 @@ export type ConsoleListNodePoolsQuery = {
clusterName: string;
creationTime: any;
displayName: string;
stateful: boolean;
markedForDeletion?: boolean;
recordVersion: number;
updateTime: any;
Expand All @@ -2115,6 +2112,7 @@ export type ConsoleListNodePoolsQuery = {
cloudProvider: Github__Com___Kloudlite___Operator___Apis___Common____Types__CloudProvider;
maxCount: number;
minCount: number;
nodeLabels?: any;
aws?: {
availabilityZone: string;
nvidiaGpuEnabled: boolean;
Expand Down
Loading