fix: fixes Aws cluster and nodepool creation flow#284
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 updates to the AWS cluster and nodepool creation flow, specifically focusing on the integration of VPC information from the cluster resource into the nodepool creation process. It includes changes to GraphQL queries, constants in Go files, and the logic within the domain layer to accommodate the new VPC-related fields.
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.
- Big diff: the diff is too large to approve with confidence.
General suggestions:
- Consider reviewing the security implications of removing IAM instance profile roles in the cluster configuration. It's crucial to ensure that clusters still have the necessary permissions to access AWS resources.
- Given the significant changes, especially around the handling of VPC and subnet IDs, it would be beneficial to ensure comprehensive testing around these areas. While individual comments have suggested adding tests, an overarching suggestion is to focus on integration and end-to-end tests that cover the new flow comprehensively.
- The removal of certain fields like
ImageIdandImageSSHUsernamefrom the cluster and nodepool configurations suggests a shift in how AWS resources are managed. It might be helpful to document the rationale behind these changes for future reference and to aid in understanding the new approach. - The changes made in GraphQL queries and models indicate an evolution in the data structure and how data is managed. It's important to verify that these changes are well-documented, especially for teams that rely on these interfaces for integration.
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? ✨
| case ct.CloudProviderAWS: | ||
| { | ||
|
|
||
| awsSubnetID := cluster.Spec.AWS.VPC.GetSubnetId(nodepool.Spec.AWS.AvailabilityZone) |
There was a problem hiding this comment.
issue (llm): I noticed that the logic for selecting the AWS subnet ID based on the availability zone has been added. However, there are no unit tests covering this new logic. It's crucial to ensure that the correct subnet ID is selected for the given availability zone, especially considering the error handling when no subnet is configured for the provided availability zone. Could you add unit tests to cover these scenarios?
| return &clustersv1.AWSClusterConfig{ | ||
| Region: cluster.Spec.AWS.Region, | ||
| K3sMasters: clustersv1.AWSK3sMastersConfig{ | ||
| ImageId: d.env.AWSAMI, |
There was a problem hiding this comment.
question (llm): I see that the ImageId and ImageSSHUsername fields have been removed from the AWSClusterConfig and AWSNodePoolConfig structures. It's important to ensure that removing these fields does not affect the cluster and node pool creation process on AWS. Are there integration tests or end-to-end tests that validate the cluster and node pool creation flow with these changes? If not, it would be beneficial to add such tests to ensure the functionality remains intact.
| MessageTimestamp: opts.MessageTimestamp, | ||
| XPatch: repos.Document{ | ||
| fc.ClusterSpecOutput: cluster.Spec.Output, | ||
| fc.ClusterSpecAwsVpc: cluster.Spec.AWS.VPC, |
There was a problem hiding this comment.
suggestion (llm): The update to include ClusterSpecAwsVpc in the XPatch document is a significant change. It would be prudent to have tests verifying that the VPC information is correctly updated in the cluster resource. This ensures that any downstream processes relying on the VPC information can function correctly. Could you add tests to verify the correct behavior of this update operation?
| NodePools map[string]interface{} `json:"nodePools,omitempty"` | ||
| Region string `json:"region"` | ||
| SpotNodePools map[string]interface{} `json:"spotNodePools,omitempty"` | ||
| Vpc *GithubComKloudliteOperatorApisClustersV1AwsVPCParams `json:"vpc,omitempty"` |
There was a problem hiding this comment.
suggestion (llm): The addition of the Vpc field to the GithubComKloudliteOperatorApisClustersV1AWSClusterConfig struct is a key change. It's essential to ensure that this new field is correctly handled in all relevant GraphQL operations. Are there tests that cover the serialization and deserialization of this field in GraphQL queries and mutations? Adding such tests would help maintain the robustness of the GraphQL API.
| nodepool.Spec.AWS = &clustersv1.AWSNodePoolConfig{ | ||
| ImageId: d.env.AWSAMI, | ||
| ImageSSHUsername: "ubuntu", | ||
| VPCId: cluster.Spec.AWS.VPC.ID, |
There was a problem hiding this comment.
suggestion (llm): The change to use VPCId and VPCSubnetID from the cluster's VPC configuration is crucial for ensuring that node pools are created with the correct networking setup. It would be beneficial to have integration tests that validate the node pool creation process with these VPC settings, especially in scenarios where the VPC or subnet IDs are invalid or not found. Such tests would help catch potential issues early in the development process.
…in-vpc fix: fixes Aws cluster and nodepool creation flow
1 participant
Description