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

chore: support contributing to the AWS CDK using alternative container clients #23855

Merged
merged 5 commits into from Jan 30, 2023

Conversation

mrgrain
Copy link
Contributor

@mrgrain mrgrain commented Jan 26, 2023

The AWS CDK uses docker for a feature subset related to building assets.
For users, we already support the CDK_DOCKER env variable to change the container client at runtime.
However contributing to the AWS CDK currently still requires a contributor to have docker installed.

This PR extends the support for the CDK_DOCKER env variable to contributor tooling.
For example one can now run:

CDK_DOCKER=finch ./scripts/foreach.sh "yarn build"
CDK_DOCKER=finch ./scripts/foreach.sh "yarn test"

When using finch, the following packages do still fail tests, due to known feature gaps:

@aws-cdk/aws-ec2
@aws-cdk/aws-lambda
@aws-cdk/aws-lambda-go
@aws-cdk/aws-lambda-python
@aws-cdk/aws-lambda-nodejs
@aws-cdk/aws-s3-deployment

All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Jan 26, 2023

@mrgrain mrgrain marked this pull request as draft January 26, 2023 19:54
@aws-cdk-automation aws-cdk-automation requested a review from a team January 26, 2023 19:54
@github-actions github-actions bot added the p2 label Jan 26, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 26, 2023
@mrgrain mrgrain force-pushed the mrgrain/chore/build-with-finch branch from 16e6068 to dc4d84e Compare January 27, 2023 13:42
@mrgrain mrgrain marked this pull request as ready for review January 27, 2023 15:52
@MrArnoldPalmer
Copy link
Contributor

I believe this will resolve #16209, linking for posterity.

@ningziwen
Copy link
Member

The following packages do currently fail tests, presumingly due to some incompatibility of finch with Docker features. What exactly, will require further investigation.

Is it possible to log the finch commands when they fail? We can probably get a full list of the imcompatible gap from this.

@ningziwen
Copy link
Member

Is the "build" in this PR mean the build of CDK binary? (as I saw you run yarn build for each in the PR description)

If we add CDK_DOCEKR to yarn build, does it mean CDK will have different binaries for each container client?

@RomainMuller
Copy link
Contributor

Is the "build" in this PR mean the build of CDK binary? (as I saw you run yarn build for each in the PR description)

If we add CDK_DOCEKR to yarn build, does it mean CDK will have different binaries for each container client?

There is no such thing as a CDK binary... Anyhow, this is a runtime-change... the use of yarn build here was to validate that the tests all still pass when using finch.

@mrgrain this makes me think we should fast-follow with a PR to ensure we run integration tests with both Docker & Finch (at least the container-using ones).

@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2023
@mrgrain mrgrain changed the title chore: support build with alternative container clients chore: support contributing to the CDK using alternative container clients Jan 30, 2023
@mrgrain mrgrain changed the title chore: support contributing to the CDK using alternative container clients chore: support contributing to the AWS CDK using alternative container clients Jan 30, 2023
@mrgrain
Copy link
Contributor Author

mrgrain commented Jan 30, 2023

I believe this will resolve #16209, linking for posterity.

@MrArnoldPalmer I don't think it does. #16209 talks about how podman isn't fully compatible with docker and so a few docker commands we currently use might need adjustment to be podman compatible. It's similar to how finch has feature gaps.

@mrgrain mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

lgtm

@mergify
Copy link
Contributor

mergify bot commented Jan 30, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jan 30, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@MrArnoldPalmer
Copy link
Contributor

@mrgrain I took it as "provide a way to swap out docker for something else" whether that something else is 100% compatible with all the things we need it to do is kinda hard to control. For podman specifically we can test it, but regardless you're right lets keep it open for now.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: eb9133f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit a6fa5b8 into aws:main Jan 30, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 30, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

elif [ "$container_client" == "finch" ]; then
check_which $container_client $finch_min

# Make sure docker is running
Copy link
Member

Choose a reason for hiding this comment

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

"Make sure finch is running"

@mrgrain mrgrain deleted the mrgrain/chore/build-with-finch branch July 26, 2023 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants