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.

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

add aws connection method func #396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add aws connection method func #396

wants to merge 1 commit into from

Conversation

refaelm92
Copy link
Contributor

@refaelm92 refaelm92 commented Nov 26, 2024

PR Type

enhancement


Description

  • Introduced a new AWSConnectionMethod type to define AWS connection methods.
  • Added constants CloudIAMRole and Credentials to specify connection methods.
  • Implemented the GetConnectionMethod function in the AWSImageRegistry struct to determine the connection method based on the presence of RoleARN.

Changes walkthrough 📝

Relevant files
Enhancement
registrymethods.go
Add AWS connection method functionality                                   

armotypes/registrymethods.go

  • Added a new type AWSConnectionMethod to represent connection methods.
  • Introduced constants CloudIAMRole and Credentials for connection
    methods.
  • Implemented GetConnectionMethod function to determine the AWS
    connection method.
  • +14/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Validation Order
    The GetConnectionMethod function may be called before Validate(), which could lead to incorrect connection method determination if authentication data is missing

    Type Documentation
    The new AWSConnectionMethod type and its constants lack documentation comments explaining their purpose and usage

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Improve input validation to ensure credential pairs are complete when provided

    Add validation to ensure that both access key and secret key are provided together
    when using credentials authentication method.

    armotypes/registrymethods.go [85-87]

    -if (aws.AccessKeyID == "" || aws.SecretAccessKey == "") && aws.RoleARN == "" {
    -    return errors.New("missing authentication data")
    +if aws.RoleARN == "" {
    +    if aws.AccessKeyID == "" && aws.SecretAccessKey == "" {
    +        return errors.New("missing authentication data")
    +    }
    +    if aws.AccessKeyID == "" || aws.SecretAccessKey == "" {
    +        return errors.New("both access key and secret key must be provided")
    +    }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion significantly improves input validation by providing more specific error messages and ensuring both credentials are provided together, which prevents potential authentication issues.

    9
    General
    Explicitly type constant values to ensure type safety and prevent potential reordering issues

    Add explicit type values to the constants to prevent potential issues if the
    constant order changes or new constants are added between them.

    armotypes/registrymethods.go [93-96]

     const (
    -    CloudIAMRole = iota
    -    Credentials
    +    CloudIAMRole AWSConnectionMethod = iota
    +    Credentials AWSConnectionMethod = iota
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Explicitly typing the constants with AWSConnectionMethod improves type safety and makes the code more maintainable, especially if more constants are added in the future.

    7

    💡 Need additional feedback ? start a PR chat

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant