fix(web/nodepool): remove stateful type as extra field #112
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces a significant change in how nodepools' statefulness is represented within the system. Instead of using a direct boolean flag, the statefulness is now indicated through a label within the nodeLabels map. This change affects various parts of the system, including the nodepool handling logic, GraphQL queries, and the underlying data model. The modifications ensure that the statefulness of nodepools is more flexibly managed and integrated into the broader labeling system used across the platform.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Given the shift from a boolean flag to a label-based representation for nodepool statefulness, it's crucial to ensure comprehensive coverage in testing. This includes not only direct tests for the presence and correctness of the
stateful/statelesslabels but also broader integration tests to confirm that this new approach interacts seamlessly with existing functionalities. - Consider adding more detailed documentation or comments within the code to explain the rationale behind this change. This could help future maintainers understand the decision-making process and the specific benefits of using labels over direct boolean flags for representing statefulness.
- Review the potential impact of this change on existing nodepools and ensure that there's a clear migration path or compatibility layer for nodepools defined under the old system. This might involve temporary support for the old
statefulflag alongside the new label-based approach.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -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 | |||
There was a problem hiding this comment.
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.
| @@ -155,9 +156,11 @@ const Root = (props: IDialog) => { | |||
| maxCount: Number.parseInt(val.maximum, 10), | |||
| minCount: Number.parseInt(val.minimum, 10), | |||
| cloudProvider: 'aws', | |||
| nodeLabels: { | |||
There was a problem hiding this comment.
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.
| @@ -155,9 +156,11 @@ const Root = (props: IDialog) => { | |||
| maxCount: Number.parseInt(val.maximum, 10), | |||
| minCount: Number.parseInt(val.minimum, 10), | |||
| cloudProvider: 'aws', | |||
| nodeLabels: { | |||
There was a problem hiding this comment.
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.
| @@ -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']; | |||
There was a problem hiding this comment.
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.
| @@ -79,6 +78,7 @@ export const nodepoolQueries = (executor: IExecutor) => ({ | |||
| cloudProvider | |||
| maxCount | |||
| minCount | |||
| nodeLabels | |||
There was a problem hiding this comment.
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.
fix(web/nodepool): remove stateful type as extra field
fix(web/nodepool): remove stateful type as extra field
fix(web/nodepool): remove stateful type as extra field
No description provided.