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 vpc and architectures option to the creation of lambda from source #4723

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

nnnkkk7
Copy link
Contributor

@nnnkkk7 nnnkkk7 commented Dec 17, 2023

What this PR does / why we need it:
I added vpc and architectures optional configuration to creation of lambda from source code as they did not exist.
Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

@khanhtc1202
Copy link
Member

/review

Copy link
Contributor

## PR Analysis
  ### Main theme
    Enhancement
  ### PR summary
    The PR adds support for function architectures and VPC configuration when creating AWS Lambda functions through a custom client wrapper.
  ### Type of PR
    Enhancement

## PR Feedback:
  ### General suggestions
    The changes introduced in the PR are logical enhancements to the AWS Lambda client functionality. It adds the ability to specify function architectures and VPC configurations when creating a new lambda function, which are both important features for lambdas that need to operate within a VPC or require specific architectures. Ensuring the provided values match AWS expected formats should be accounted for to prevent runtime issues.
    
  ### Code feedback
    - relevant file: "pkg/app/piped/platformprovider/lambda/client.go"
      suggestion: "Consider validating the supported architectures before appending them to the 'architectures' slice to ensure the supplied architecture names are valid AWS Lambda architecture types. This can prevent the creation of a Lambda function with an invalid architecture and result in a better user experience by providing immediate feedback to the caller if an unsupported architecture is specified. Use 'types.Architecture' enumeration to check validity. [important]"
      relevant line: "+			architectures = append(architectures, types.Architecture(arch.Name))"
    - relevant file: "pkg/app/piped/platformprovider/lambda/client.go"
      suggestion: "For better performance, preallocate the 'architectures' slice with the exact required capacity since the size is known from 'fm.Spec.Architectures'. This can be achieved by using 'make([]types.Architecture, 0, len(fm.Spec.Architectures))'. [medium]"
      relevant line: "+		var architectures []types.Architecture"
    - relevant file: "pkg/app/piped/platformprovider/lambda/client.go"
      suggestion: "Ensure 'SecurityGroupIds' and 'SubnetIds' slices in 'types.VpcConfig' are properly validated for empty values or malformed inputs before attempting to create the Lambda function. This can prevent runtime API errors and improve overall robustness. [important]"
      relevant line: "+			SecurityGroupIds: fm.Spec.VPCConfig.SecurityGroupIDs,"
      
  ### Security concerns:
    no

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (847b578) 30.84% compared to head (4e1785c) 30.79%.

Files Patch % Lines
pkg/app/piped/platformprovider/lambda/client.go 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4723      +/-   ##
==========================================
- Coverage   30.84%   30.79%   -0.06%     
==========================================
  Files         221      221              
  Lines       25993    26005      +12     
==========================================
- Hits         8018     8008      -10     
- Misses      17325    17347      +22     
  Partials      650      650              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nnnkkk7 nnnkkk7 requested a review from khanhtc1202 December 17, 2023 16:11
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Thanks 🙌

@khanhtc1202
Copy link
Member

@nnnkkk7 the DCO check is failed, please resign to the commit 🙏

Signed-off-by: nnnkkk7 <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
@nnnkkk7 nnnkkk7 force-pushed the fix/CreateFunctionFromSource branch from 9571c35 to 4e1785c Compare December 18, 2023 04:17
@khanhtc1202 khanhtc1202 enabled auto-merge (squash) December 18, 2023 08:33
@khanhtc1202
Copy link
Member

@ffjlabo could you check this?

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

Thank you for fixing 🚀

@khanhtc1202 khanhtc1202 merged commit c16bca3 into pipe-cd:master Dec 18, 2023
11 of 13 checks passed
nnnkkk7 added a commit to nnnkkk7/pipecd that referenced this pull request Feb 1, 2024
pipe-cd#4723)

* add config option to CreateFunctionFromSource

Signed-off-by: nnnkkk7 <[email protected]>

* preallocate the 'architectures' slice

Signed-off-by: nnnkkk7 <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>

---------

Signed-off-by: nnnkkk7 <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
@github-actions github-actions bot mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants