feat(apps/infra): fixes Check AWS Access implementation#261
Conversation
given cloudformation is completed
![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 enhancements to the AWS access validation logic within the infrastructure domain. It extends the functionality of the validateAWSAssumeRole function by incorporating additional parameters for instance profile name and CloudFormation stack name, enabling more comprehensive validation checks. Additionally, it modifies a GraphQL configuration and removes an obsolete validator file, streamlining the project structure.
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.
- Files deleted: Sourcery does not currently approve diffs with deleted files.
General suggestions:
- Consider parameterizing the AWS region or retrieving it from a configuration setting to enhance the function's adaptability to different deployment environments, rather than hardcoding it.
- Review the error messaging for clarity and accuracy, especially in scenarios where AWS roles or CloudFormation stacks are not found, to ensure they accurately reflect the error state and facilitate debugging.
- Evaluate the possibility of refactoring the
validateAWSAssumeRolefunction to adhere to the single responsibility principle, potentially by separating the validation of the assume role and the CloudFormation stack into distinct functions. - Ensure that changes to configuration files, such as the provider name in the GraphQL configuration, are intentional and reflected across all dependent configurations and documentation to avoid potential integration issues.
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? ✨
| func (d *domain) validateAWSAssumeRole(_ context.Context, paramExternalId string, roleARN string) error { | ||
| func (d *domain) validateAWSAssumeRole(_ context.Context, paramExternalId string, roleARN string, instanceProfileName string, cfStackName string) error { | ||
| sess, err := session.NewSession() | ||
| sess.Config.Region = aws.String("ap-south-1") |
There was a problem hiding this comment.
suggestion (llm): Hardcoding the AWS region to 'ap-south-1' may limit the flexibility of this function. Consider parameterizing the region or retrieving it from a configuration setting to enhance the function's adaptability to different deployment environments.
|
|
||
| if resp.AssumedRoleUser.Arn != nil { | ||
| return nil | ||
| if resp.AssumedRoleUser == nil || resp.AssumedRoleUser.Arn == nil { |
There was a problem hiding this comment.
suggestion (llm): The error message for a missing AWS assume role does not specify whether the role or the ARN is missing. Providing a more detailed error message could help with debugging.
| } | ||
| } | ||
|
|
||
| if !stackFound { |
There was a problem hiding this comment.
suggestion (llm): The error message 'waiting for cloudformation stack to be created' might be misleading as it suggests an action (waiting) rather than the actual error state (stack not found). Consider rephrasing to clearly indicate that the stack was not found.
| } | ||
|
|
||
| if err := d.validateAWSAssumeRole(ctx, psecret.AWS.CfParamExternalID, psecret.AWS.GetAssumeRoleRoleARN()); err != nil { | ||
| if err := d.validateAWSAssumeRole(ctx, psecret.AWS.CfParamExternalID, psecret.AWS.GetAssumeRoleRoleARN(), psecret.AWS.CfParamInstanceProfileName, psecret.AWS.CfParamStackName); err != nil { |
There was a problem hiding this comment.
suggestion (llm): Adding parameters 'instanceProfileName' and 'cfStackName' to 'validateAWSAssumeRole' significantly increases the function's responsibilities. Consider refactoring to maintain single responsibility principle, possibly by splitting the validation of the assume role and the cloud formation stack into separate functions.
feat(apps/infra): fixes Check AWS Access implementation
1 participant
Resolves kloudlite/kloudlite#19