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

fix(platforms): unable to perform awscdk context lookups #6286

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Apr 21, 2024

In order to fix #6279, we need synthesis to be triggered by the CDK CLI instead of directly from the Wing CLI.

This is actually really easy to do. All you need is to create a cdk.json file (or use one from cdk init) and modify the app and the output options like so:

{
  "app": "CDK_STACK_NAME=MyStack wing compile --platform @winglang/platform-awscdk main.w",
  "output": "target/main.awscdk"
}

Then, you can simply use cdk deploy, cdk diff, cdk synth as if it was a normal CDK app. No need to explicitly interact with the Wing CLI in this case.

To allow context lookups, we need to support specifying the stack's AWS environment (account/region), so two new environment variables have been added: CDK_AWS_ACCOUNT and CDK_AWS_REGION. These can be set together with CDK_STACK_NAME in the cdk.json file mentioned above.

Checklist

  • Title matches Winglang's style guide
  • Description explains motivation and solution
  • Tests added (always)
  • Docs updated (only required for features)
  • Added pr/e2e-full label if this feature requires end-to-end testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

In order to fix #6279, we need synthesis to be triggered by the CDK CLI instead of directly from the Wing CLI.

This is actually really easy to do. All you need is to create a `cdk.json` file (or use one from `cdk init`) and modify the `app` and the `output` options like so:

```json
{
  "app": "CDK_STACK_NAME='MyStack' wing compile --platform @winglang/platform-awscdk main.w",
  "output": "target/main.awscdk"
}
```

Then, you can simply use `cdk deploy`, `cdk diff`, `cdk synth` as if it was a normal CDK app. No need to explicitly interact with the Wing CLI in this case.

To allow context lookups, we need to support specifying the stack's AWS environment (account/region), so two new environment variables have been added: `CDK_AWS_ACCOUNT` and `CDK_AWS_REGION`. These can be set together with `CDK_STACK_NAME` in the `cdk.json` file mentioned above.
@eladb eladb requested a review from a team as a code owner April 21, 2024 20:05
Copy link

Thanks for opening this pull request! 🎉
Please consult the contributing guidelines for details on how to contribute to this project.
If you need any assistence, don't hesitate to ping the relevant owner over Slack.

Topic Owner
Wing SDK and utility APIs @chriscbr
Wing Console @ainvoner, @skyrpex, @polamoros
JSON, structs, primitives and collections @hasanaburayyan
Platforms and plugins @hasanaburayyan
Frontend resources (website, react, etc) @tsuf239
Language design @chriscbr
VSCode extension and language server @markmcculloh
Compiler architecture, inflights, lifting @yoav-steinberg
Wing Testing Framework @tsuf239
Wing CLI @markmcculloh
Build system, dev environment, releases @markmcculloh
Library Ecosystem @chriscbr
Documentation @hasanaburayyan
SDK test suite @tsuf239
Examples @hasanaburayyan
Wing Playground @eladcon

@eladb eladb changed the title fix(awscdk): unable to perform awscdk context lookups fix(platforms): unable to perform awscdk context lookups Apr 21, 2024
@monadabot
Copy link
Contributor

Benchmarks

Comparison to Baseline ⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜
Benchmark Before After Change
version 60ms±0.55 60ms±0.86 0ms (-0.4%)⬜
functions_1.test.w -t sim 393ms±5.44 400ms±7.21 +7ms (+1.73%)⬜
functions_1.test.w -t tf-aws 820ms±10.38 822ms±15.11 +2ms (+0.23%)⬜
jsii_big.test.w -t sim 2785ms±18.87 2806ms±24.55 +22ms (+0.78%)⬜
jsii_big.test.w -t tf-aws 3008ms±32.46 3007ms±14.43 0ms (-0.01%)⬜
functions_10.test.w -t sim 485ms±12.28 482ms±9.63 -4ms (-0.74%)⬜
functions_10.test.w -t tf-aws 1999ms±14.99 2029ms±18.99 +29ms (+1.47%)⬜
jsii_small.test.w -t sim 364ms±3.28 364ms±3.61 0ms (-0.02%)⬜
jsii_small.test.w -t tf-aws 600ms±3.28 601ms±3.46 +1ms (+0.19%)⬜
empty.test.w -t sim 359ms±4.9 358ms±3.78 -1ms (-0.15%)⬜
empty.test.w -t tf-aws 590ms±7.52 595ms±4.01 +5ms (+0.93%)⬜
hello_world.test.w -t sim 413ms±8.64 404ms±10.97 -9ms (-2.22%)⬜
hello_world.test.w -t tf-aws 1525ms±20.47 1510ms±7.92 -16ms (-1.02%)⬜

⬜ Within 1.5 standard deviations
🟩 Faster, Above 1.5 standard deviations
🟥 Slower, Above 1.5 standard deviations

Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI.

Results
name mean min max moe sd
version 60ms 58ms 62ms 1ms 1ms
functions_1.test.w -t sim 400ms 388ms 424ms 7ms 10ms
functions_1.test.w -t tf-aws 822ms 801ms 868ms 15ms 21ms
jsii_big.test.w -t sim 2806ms 2765ms 2869ms 25ms 34ms
jsii_big.test.w -t tf-aws 3007ms 2973ms 3041ms 14ms 20ms
functions_10.test.w -t sim 482ms 463ms 501ms 10ms 13ms
functions_10.test.w -t tf-aws 2029ms 1995ms 2091ms 19ms 27ms
jsii_small.test.w -t sim 364ms 357ms 371ms 4ms 5ms
jsii_small.test.w -t tf-aws 601ms 595ms 610ms 3ms 5ms
empty.test.w -t sim 358ms 348ms 365ms 4ms 5ms
empty.test.w -t tf-aws 595ms 586ms 603ms 4ms 6ms
hello_world.test.w -t sim 404ms 391ms 439ms 11ms 15ms
hello_world.test.w -t tf-aws 1510ms 1494ms 1527ms 8ms 11ms
Last Updated (UTC) 2024-04-21 20:14

eladb added a commit to winglang/examples that referenced this pull request Apr 21, 2024
The *new* best practice as described in winglang/wing#6286 is to configure a `cdk.json` file to use `wing compile` as the CDK app.

This standardizes the usage through the CDK CLI and enables all CDK features like context lookups, etc.
@eladb eladb requested a review from MarkMcCulloh April 21, 2024 20:45
Copy link
Contributor

@hasanaburayyan hasanaburayyan left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this one @eladb, I think we should ship this for necessity. I do want to continue the conversation around this flow and if we can/should make this more "wing" native feeling. Ill make some notes and add a topic proposal to a team time.

@eladb
Copy link
Contributor Author

eladb commented Apr 21, 2024

continue the conversation

Happy to continue the discussion.

The way I see it, the Wing CLI is currently focused on compilation, local development and testing. Deployment is out of scope and ideally should be as idiomatic as possible (and as standard as possible) to the target platform.

Which is why I think that for CDK users this is better and more idiomatic DX because that's exactly how they work with any CDK language, and I don't think Wing should be different.

If/when we decide that the Wing CLI will own deployment, we will need hooks at the platform level to support it and that's where we will need to execute the CDK CLI (or the Terraform CLI), so context lookups will be solved there.

There's a lot of value in not dealing with the huge surface area of these provisioning tools right now.

Copy link
Contributor

mergify bot commented Apr 21, 2024

Thanks for contributing, @eladb! This PR will now be added to the merge queue, or immediately merged if eladb/awscdk-synth is up-to-date with main and the queue is empty.

@mergify mergify bot merged commit ddd52cb into main Apr 21, 2024
16 of 18 checks passed
@mergify mergify bot deleted the eladb/awscdk-synth branch April 21, 2024 21:26
@hasanaburayyan
Copy link
Contributor

The only difference from the current experience is that instead of doing cdk deploy --app target/main.awscdk you just run cdk deploy. It's way more idiomatic and much nicer from every aspect as far as I am concerned.

You know, when I first started my line of thinking here, my mind thought that the current deployment modus operandi was to run aws cloudformation create-stack as the best way to deploy the output artifacts. But this is awscdk target not cf-aws so shame on me.

@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.71.4.

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.

Unable to perform lookups in awscdk based platforms
4 participants