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.

Sign up for GitHub

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

Jump to bottom

feat(lib): Asset construct #698

Merged
merged 30 commits into from
May 26, 2021
Merged

feat(lib): Asset construct #698

merged 30 commits into from
May 26, 2021

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented May 10, 2021 •
edited
Loading

Closes #648

@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch from 7e65b1b to d2fae67 Compare May 11, 2021 12:42
@danieldreier danieldreier added this to the v0.4 milestone May 11, 2021
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch 6 times, most recently from eab786c to 70afb6d Compare May 17, 2021 12:32
@DanielMSchmidt DanielMSchmidt changed the title WIP: Define asset construct Asset construct May 17, 2021
@DanielMSchmidt DanielMSchmidt marked this pull request as ready for review May 17, 2021 12:34
skorfmann
skorfmann reviewed May 17, 2021
View reviewed changes
Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

Left a few comments, but looks very good already 👍

packages/cdktf/lib/terraform-asset.ts Outdated Show resolved Hide resolved
docs/working-with-cdk-for-terraform/terraform-assets.md Outdated Show resolved Hide resolved
test/python/asset/cdktf.json Outdated Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt requested a review from skorfmann May 17, 2021 17:27
@DanielMSchmidt DanielMSchmidt requested a review from jsteinich May 18, 2021 16:43
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch from f1441e2 to 582ce08 Compare May 18, 2021 19:37
jsteinich
jsteinich requested changes May 19, 2021
View reviewed changes
Copy link
Collaborator

@jsteinich jsteinich left a comment

Choose a reason for hiding this comment

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

Looks to be on the right track, just a few things to clean up.

I think we'll also want to do a follow-up PR to make local modules automatically use assets.

packages/cdktf/lib/terraform-asset.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-stack.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/private/fs.ts Show resolved Hide resolved
test/typescript/terraform-cloud/main.ts Outdated Show resolved Hide resolved
test/python/asset/main.py Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-asset.ts Show resolved Hide resolved
packages/cdktf/lib/terraform-asset.ts Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt requested a review from jsteinich May 19, 2021 13:49
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch from 7d8f505 to 20942d2 Compare May 19, 2021 15:42
jsteinich
jsteinich requested changes May 20, 2021
View reviewed changes
Copy link
Collaborator

@jsteinich jsteinich left a comment

Choose a reason for hiding this comment

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

Just a couple of small comments.
Also, make sure to fix the tests and add a link to the new documentation page to the readme.

packages/cdktf/lib/terraform-asset.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-asset.ts Outdated Show resolved Hide resolved
@skorfmann skorfmann mentioned this pull request May 20, 2021
Closed
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch from ab669a1 to aeb8bf7 Compare May 20, 2021 10:30
skorfmann
skorfmann reviewed May 20, 2021
View reviewed changes
docs/working-with-cdk-for-terraform/terraform-assets.md Outdated Show resolved Hide resolved
packages/cdktf/lib/private/fs.ts Show resolved Hide resolved
packages/cdktf/lib/terraform-asset.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-asset.ts Outdated Show resolved Hide resolved
test/typescript/terraform-cloud/main.ts Outdated Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch 8 times, most recently from 5545a8b to 200f28d Compare May 25, 2021 12:37
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch 10 times, most recently from 3e05b79 to 93f01e7 Compare May 26, 2021 11:36
@DanielMSchmidt DanielMSchmidt force-pushed the feat-648-asset-construct branch from 93f01e7 to 771dfab Compare May 26, 2021 12:09
@DanielMSchmidt DanielMSchmidt mentioned this pull request May 26, 2021
Closed
skorfmann
skorfmann approved these changes May 26, 2021
View reviewed changes
Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

Awesome stuff! 🎉 Can't wait to use it :)

@DanielMSchmidt DanielMSchmidt changed the title Asset construct feat(lib): Asset construct May 26, 2021
@DanielMSchmidt DanielMSchmidt merged commit 73b13ff into main May 26, 2021
@DanielMSchmidt DanielMSchmidt deleted the feat-648-asset-construct branch May 26, 2021 13:01
@skorfmann skorfmann mentioned this pull request May 27, 2021
Merged
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skorfmann skorfmann skorfmann approved these changes

@jsteinich jsteinich Awaiting requested review from jsteinich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4
Development

Successfully merging this pull request may close these issues.

Asset Construct
4 participants
@DanielMSchmidt @skorfmann @jsteinich @danieldreier