Lambda cloud provider #1347

khanhtc1202 · 2021-01-05T16:09:19Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/cloudprovider/lambda/lambda.go

pkg/app/piped/cloudprovider/lambda/client.go

pipecd-bot · 2021-01-05T16:15:44Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

examples/lambda/simple/function.yaml

pipecd-bot · 2021-01-06T02:28:00Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

pkg/app/piped/cloudprovider/lambda/function.go

pipecd-bot · 2021-01-06T06:09:13Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

pipecd-bot · 2021-01-06T06:31:14Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

khanhtc1202 · 2021-01-06T06:31:44Z

pkg/config/piped.go

+	// The Amazon Resource Name (ARN) of the function's execution role.
+	Role string `json:"role"`
+	// Path to the shared credentials file.
+	//
+	// Piped attempts to retrieve credentials in the following order:
+	// 1. from the environment variables. Available environment variables are:
+	//   - AWS_ACCESS_KEY_ID or AWS_ACCESS_KEY
+	//   - AWS_SECRET_ACCESS_KEY or AWS_SECRET_KEY
+	// 2. from the given credentials file.
+	// 3. from the EC2 Instance Role
+	CredentialsFile string `json:"credentialsFile"`


[rfc] not sure this Role configuration should be part of CloudProviderLambdaConfig or FunctionManifest. In my opinion, one lambda cloud provider should strict Role and CredentialsFile config at the same place (since they both for authorization purpose) 👀 What do you think? 🤔

FunctionManifest means the configuration of kind: LambdaFunction in the config-repo?
If so, I think it should keep in the PiepdConfig as is because this should be configured by a Piped operator.

FunctionManifest means the configuration of kind: LambdaFunction in the config-repo?

Yes, it is 👍

pipecd-bot · 2021-01-06T06:54:13Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

pkg/app/piped/cloudprovider/lambda/client.go

pipecd-bot · 2021-01-06T07:18:58Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

pkg/app/piped/cloudprovider/lambda/client.go

nghialv · 2021-01-06T07:48:10Z

pkg/app/piped/cloudprovider/lambda/function.go

+		return FunctionManifest{}, err
+	}
+
+	imageURI, ok, err := unstructured.NestedString(obj.Object, "spec", "template", "spec", "image")


I am not sure why we should still use this unstructured package to parse the manifest.
We can parse it directly from the YAML by updating the manifest struct to be like

https://github.com/kubernetes/api/blob/master/apps/v1/types.go#L254-L262

In case of keep nested struct as the current sample function.yaml we need to define all sub struct to be able to unmarshal yaml content into FunctionManifest struct, I think it the cons 😅
Should we simplify the current LambdaFunction manifest 👀

apiVersion: pipecd.dev/v1beta1 kind: LambdaFunction metadata: name: SimpleFunction spec: template: metadata: tags: app: simple spec: image: ecr.ap-northeast-1.amazonaws.com/lambda-test:v0.0.1

Yes. I think we can simplify it. (Just follow our convention in the config package, not Kubernetes')

apiVersion: pipecd.dev/v1beta1 kind: LambdaFunction spec: name: SimpleFunction image: ecr.ap-northeast-1.amazonaws.com/lambda-test:v0.0.1 tags: app: simple

got it, thank you 👍

pipecd-bot · 2021-01-06T08:13:58Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/cloudprovider/lambda/function.go

pipecd-bot · 2021-01-06T12:29:43Z

Code coverage for golang is 34.01%. This pull request does not change code coverage.

nghialv · 2021-01-06T13:25:09Z

pkg/app/piped/cloudprovider/lambda/function.go

+
+const (
+	versionV1Beta1       = "pipecd.dev/v1beta1"
+	FunctionManifestKind = "LambdaFunction"


nit: Do we need to expose this variable?

nghialv · 2021-01-06T13:26:19Z

pkg/app/piped/cloudprovider/lambda/function.go

+	return nil
+}
+
+func loadFunctionManifest(path string) (FunctionManifest, error) {


Could you add some tests for this function?

sorry I forgot to commit that 🙏 actually, I wrote tests for the parseFunctionManifest instead 👀

pipecd-bot · 2021-01-06T13:52:13Z

Code coverage for golang is 33.85%. This pull request decreases coverage by -0.16%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/lambda/client.go	newClient	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/client.go	client.Apply	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/function.go	FunctionManifest.validate	`--`	`71.43%`	`+71.43%`
pkg/app/piped/cloudprovider/lambda/function.go	FunctionManifestSpec.validate	`--`	`80.00%`	`+80.00%`
pkg/app/piped/cloudprovider/lambda/function.go	loadFunctionManifest	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/function.go	parseFunctionManifest	`--`	`83.33%`	`+83.33%`
pkg/app/piped/cloudprovider/lambda/function.go	DecideRevisionName	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/function.go	FindImageTag	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/function.go	parseContainerImage	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/lambda.go	LoadFunctionManifest	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/lambda.go	registry.Client	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/lambda/lambda.go	DefaultRegistry	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-01-07T01:20:32Z

pkg/app/piped/cloudprovider/lambda/function.go

+// FunctionManifestSpec contains configuration for LambdaFunction.
+type FunctionManifestSpec struct {
+	Name     string            `json:"name"`
+	ImageURI string            `json:"image"`


It may be more appropriate to replace it with ImageRef that clearly represents a specific image that has a unique digest. But it's up to you.

I think that name may confuse ( overlap with #1358 ImageRef struct ), besides this name specific exactly what I want to be given from here ( the URI to image on ecr registry ) so I think it's good for me 😅

Okay, no objection to that 👍

nakabonne · 2021-01-07T03:01:53Z

/lgtm

nghialv · 2021-01-07T03:06:19Z

/approve

pipecd-bot · 2021-01-07T03:06:22Z

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

khanhtc1202 added 2 commits January 5, 2021 23:37

Add FunctionManifest

cf52a91

Lambda provider client

55d5884

pipecd-bot added do-not-merge/release-note-needed do-not-merge/wip size/L area/piped labels Jan 5, 2021

pipecd-bot requested review from nakabonne and nghialv January 5, 2021 16:09

pipecd-bot added release-note-none and removed do-not-merge/release-note-needed labels Jan 5, 2021

pipecd-bot reviewed Jan 5, 2021

View reviewed changes

pkg/app/piped/cloudprovider/lambda/lambda.go Outdated Show resolved Hide resolved

pkg/app/piped/cloudprovider/lambda/client.go Outdated Show resolved Hide resolved

pkg/app/piped/cloudprovider/lambda/client.go Outdated Show resolved Hide resolved

nghialv reviewed Jan 6, 2021

View reviewed changes

examples/lambda/simple/function.yaml Outdated Show resolved Hide resolved

Remove unused context on new lambda client

9d52491

nghialv reviewed Jan 6, 2021

View reviewed changes

pkg/app/piped/cloudprovider/lambda/function.go Outdated Show resolved Hide resolved

Apply implement

3cdaad6

pipecd-bot added the area/build label Jan 6, 2021

Remove unstructured field from FunctionManifest

da4a21a

khanhtc1202 marked this pull request as ready for review January 6, 2021 06:22

pipecd-bot removed the do-not-merge/wip label Jan 6, 2021

Add EOF

3840f54

khanhtc1202 commented Jan 6, 2021

View reviewed changes

local merge conflict

b03ba2a

FunctionName is required on parsing

0d5f659

nakabonne reviewed Jan 6, 2021

View reviewed changes

pkg/app/piped/cloudprovider/lambda/client.go Show resolved Hide resolved

nghialv reviewed Jan 6, 2021

View reviewed changes

pkg/app/piped/cloudprovider/lambda/client.go Outdated Show resolved Hide resolved

pkg/app/piped/cloudprovider/lambda/client.go Show resolved Hide resolved

nghialv reviewed Jan 6, 2021

View reviewed changes

Update files header

f67aa3f

Remove unused unstructured package

d92a7cd

pipecd-bot reviewed Jan 6, 2021

View reviewed changes

pkg/app/piped/cloudprovider/lambda/function.go Outdated Show resolved Hide resolved

nghialv reviewed Jan 6, 2021

View reviewed changes

Add tests

2584986

pipecd-bot added size/XL and removed size/L labels Jan 6, 2021

nakabonne reviewed Jan 7, 2021

View reviewed changes

pipecd-bot added the lgtm label Jan 7, 2021

pipecd-bot added the approved label Jan 7, 2021

pipecd-bot merged commit 3f3c888 into master Jan 7, 2021

pipecd-bot deleted the lambda-cloud-provider branch January 7, 2021 03:15

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

Lambda cloud provider #1347

Lambda cloud provider #1347

Uh oh!

Conversation

khanhtc1202 commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pipecd-bot commented Jan 5, 2021

Uh oh!

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

khanhtc1202 Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jan 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Jan 7, 2021

Uh oh!

nghialv commented Jan 7, 2021

Uh oh!

pipecd-bot commented Jan 7, 2021

Uh oh!

Reviewers

Assignees

khanhtc1202 commented Jan 5, 2021 •

edited

Loading

khanhtc1202 Jan 6, 2021 •

edited

Loading

nghialv Jan 6, 2021 •

edited

Loading

khanhtc1202 Jan 7, 2021 •

edited

Loading