winglang / wing

A programming language for the cloud ☁️ A unified programming model, combining infrastructure and runtime code into one language ⚡
https://winglang.io
Other
4.73k stars 185 forks source link

feat(sdk): adding cloud.Api to awscdk platform #4974

Closed marciocadev closed 4 months ago

marciocadev commented 7 months ago

Create the cloud.Api implementation for awscdk

Closes https://github.com/winglang/wing/issues/3783

Checklist

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

revitalbarletz commented 6 months ago

@marciocadev do you need any help here?

marciocadev commented 6 months ago

@marciocadev do you need any help here?

Hi @revitalbarletz, no need, I've already corrected it. I hadn't noticed that there were issues.

tsuf239 commented 6 months ago

Hi :) everything passes on tf-aws, but not all manual tests passed on awscdk, Can you please fix them? I ran: pnpm wing test -t libs/awscdk/lib examples/tests/sdk_tests/api/*.test.w and got:

Results:
    ✓ api/aws-api.test.w
    ✓ api/post.test.w
    ✓ api/404.test.w
    ✓ api/options.test.w
    ✓ api/delete.test.w
    ✓ api/cors.test.w
    ✓ api/get.test.w
    × api/put.test.w
    × api/patch.test.w
    × api/path_vars.test.w

Errors:
At api/put.test.w
Error: assertion failed: response.headers.get("content-type") == "application/json; charset=utf-8"
    at /var/task/index.js:29:23
    at $Closure2.handle (/var/task/index.js:30:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exports.handler (/var/task/index.js:5970:10)
At api/patch.test.w
Error: assertion failed: response.body == _id
    at /var/task/index.js:29:23
    at $Closure2.handle (/var/task/index.js:30:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exports.handler (/var/task/index.js:5936:10)
At api/path_vars.test.w
Error: assertion failed: res.status == 200
    at /var/task/index.js:24:23
    at $Closure3.handle (/var/task/index.js:25:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exports.handler (/var/task/index.js:5883:10),Error: assertion failed: res.status == 200
    at /var/task/index.js:24:23
    at $Closure4.handle (/var/task/index.js:25:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exports.handler (/var/task/index.js:5883:10),Error: assertion failed: res.status == 200
    at /var/task/index.js:24:23
    at $Closure5.handle (/var/task/index.js:25:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exports.handler (/var/task/index.js:5883:10),Error: assertion failed: res.status == 200
    at /var/task/index.js:25:23
    at $Closure6.handle (/var/task/index.js:26:15)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async exports.handler (/var/task/index.js:5901:10)

Tests 6 failed | 7 passed (13)
Test Files 3 failed | 7 passed (10)
marciocadev commented 6 months ago

Hi @tsuf239

Thank you for reviewing this PR

To run the tests, I had to update the project with the latest commits. To test it, I had to install both @winglang/platform-awscdk and @winglang/sdk locally in the directory

My package.json looks like this, and then the tests run normally. I believe that if you run the CDK tests directly in the Wing directory, the test will reference the installed version of @winglang/sdk rather than the version that was built in the project

{
  "dependencies": {
    "@winglang/platform-awscdk": "file:../../wing/dist/winglang-platform-awscdk-0.0.0.tgz",
    "@winglang/sdk": "file:../../wing/dist/winglang-sdk-0.0.0.tgz"
  }
}

Since @winglang/platform-awscdk depends on @winglang/sdk, I ran these tests separately to ensure that I was using the updated version.

In fact, I believe that the platforms are unfortunately being created with a high level of coupling. 😞

tsuf239 commented 6 months ago

I see now @marciocadev, thanks for the comment :) Before I approve I just want to make sure the way you tested the changes is valid (=testing again after deploying and publishing the platform library will yield the same result) @hasanaburayyan can you please confirm?

mergify[bot] commented 6 months ago

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

marciocadev commented 6 months ago

Sorry for the delay in response, @tsuf239

I think the PR is having issues with merging into the Main because the awscdk's cloud.Api depends on a change in the SDK (basically, I moved the CORS part that existed in tf-aws to shared-aws).

I'm considering closing this PR and opening a new one that only changes the CORS to be used for both awscdk and tf-aws, and then creating one for cloud.Api in awscdk.

Let me know what you think.

tsuf239 commented 6 months ago

I'm considering closing this PR and opening a new one that only changes the CORS to be used for both awscdk and tf-aws, and then creating one for cloud.Api in awscdk.

Let me know what you think.

ok, let's try this out, can you open an issue regarding that? I think we might have this problem with other platforms :/

tsuf239 commented 5 months ago

@marciocadev Can you maybe try to merge main in? to see if it helps?

mergify[bot] commented 5 months ago

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

tsuf239 commented 5 months ago

@mergifyio refresh

mergify[bot] commented 5 months ago

refresh

✅ Pull request refreshed

tsuf239 commented 5 months ago

https://github.com/winglang/wing/actions/runs/7314100608/job/19926464084 the mergify process thinks a mutation is needed - I am running the PR gh build action again to see if it can do the mutation

mergify[bot] commented 5 months ago

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

marciocadev commented 5 months ago

@tsuf239

I've come across some issues while exposing the api url. I'm working on resolving them, so I think it's better not to merge yet.

marciocadev commented 5 months ago

@tsuf239

I changed this pull request to draft because I believe there are issues with the AWS CDK target that are preventing this implementation from being completed yet.

I open a issue related https://github.com/winglang/wing/issues/5314

mergify[bot] commented 4 months ago

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

mergify[bot] commented 4 months ago

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

monadabot commented 4 months ago

Congrats! :rocket: This was released in Wing 0.54.63.