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 eks and ecs exec types for golang #274

Merged
merged 2 commits into from
Feb 27, 2023
Merged

add eks and ecs exec types for golang #274

merged 2 commits into from
Feb 27, 2023

Conversation

jhsinger-klotho
Copy link
Contributor

• Does any part of it require special attention?
• Does it relate to or fix any issue? closes https://github.com/klothoplatform/klotho-history/issues/469

Images are only 25-35 MB for EKS and ECS with golang.

Adding new dockerfile and doing some refactors to the expose plugin to not be lambda specific/moving that into runtime

exporting aws service names from the provider so they can be used in the runtime

Standard checks

  • Unit tests: Any special considerations? all are still passing, shouldnt need anything new since the functionality is all infra based
  • Docs: Do we need to update any docs, internal or public? not needed
  • Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working? yes

Copy link
Contributor

@gordon-klotho gordon-klotho left a comment

Choose a reason for hiding this comment

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

Nothing blocking, but some improvements in the comments


WORKDIR /usr/src/app
ENV GOOS=linux GOARCH=amd64 CGO_ENABLED=0
COPY . .
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the layer caching suboptimal - can you copy from the lambda one (where it add go.mod first before the go mod stuff, then adds the rest of the files before building).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the previous pr you had put in for the dockerfile didnt fully work until i made some of the changes which are now reflected in both spots. I can try to make the change your suggesting but if i remember correctly, it wasnt building the image as expected

Copy link
Contributor Author

@jhsinger-klotho jhsinger-klotho Feb 23, 2023

Choose a reason for hiding this comment

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

also with the way its built and go being a statically linked isnt it just building the binary itself and nothing else? so the entire contents of the image is just the binary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the layer caching is mostly for build times and can be a huge help for CI/CD cases but doesn't affect the runtime image in this case.

Comment on lines 81 to 101
newFileContent := oldFileContent

oldNodeContent := nodeToComment
newNodeContent := commentRegex.ReplaceAllString(oldNodeContent, "// $1")

//TODO: investigate correctly indenting code
dispatcherCode := fmt.Sprintf(`
// Begin - Added by Klotho
chiLambda := chiadapter.New(%s)
handler := func(ctx context.Context, req events.APIGatewayProxyRequest) (events.APIGatewayProxyResponse, error) {
return chiLambda.ProxyWithContext(ctx, req)
}
lambda.StartWithContext(context.Background(), handler)
//End - Added by Klotho`, routerName)

newNodeContent = newNodeContent + dispatcherCode
newFileContent = strings.ReplaceAll(
newFileContent,
oldNodeContent,
newNodeContent,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's a much better way to do this now with f.ReplaceNodeContent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was copy paste from the initial code, just moved it into runtime, so let me refactor a bit to clean it up

@github-actions
Copy link

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 46%
github.com/klothoplatform/klotho/pkg/annotation 23%
github.com/klothoplatform/klotho/pkg/cli 4%
github.com/klothoplatform/klotho/pkg/core 20%
github.com/klothoplatform/klotho/pkg/env_var 82%
github.com/klothoplatform/klotho/pkg/exec_unit 54%
github.com/klothoplatform/klotho/pkg/infra/kubernetes 59%
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm 39%
github.com/klothoplatform/klotho/pkg/input 72%
github.com/klothoplatform/klotho/pkg/lang 38%
github.com/klothoplatform/klotho/pkg/lang/dockerfile 0%
github.com/klothoplatform/klotho/pkg/lang/golang 39%
github.com/klothoplatform/klotho/pkg/lang/javascript 48%
github.com/klothoplatform/klotho/pkg/lang/python 63%
github.com/klothoplatform/klotho/pkg/lang/yaml 0%
github.com/klothoplatform/klotho/pkg/logging 23%
github.com/klothoplatform/klotho/pkg/multierr 95%
github.com/klothoplatform/klotho/pkg/provider/aws 49%
github.com/klothoplatform/klotho/pkg/provider/aws/resources 70%
github.com/klothoplatform/klotho/pkg/runtime 78%
github.com/klothoplatform/klotho/pkg/static_unit 33%
github.com/klothoplatform/klotho/pkg/updater 30%
github.com/klothoplatform/klotho/pkg/validation 74%
github.com/klothoplatform/klotho/pkg/yaml_util 79%
Summary 44% (4352 / 9888)

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

Successfully merging this pull request may close these issues.

2 participants