Simplify CDK templates and avoid VPC lookups during synth step #170
Conversation
|
You're too quick! ;-) Indeed it does, and that was the intended result! @gvdongen pointed out that the |
This change simplifies the CDK examples slightly, making them more general. A side effect of this is that we no longer require a VPC lookup, which needs AWS credentials - and was thus previously failing during Unknown command: "build" To see a list of supported npm commands, run: npm help in CI.
pcholakov
changed the title
pcholakov
changed the title
pcholakov
changed the title
| @@ -12,7 +14,7 @@ npm_install_check $PROJECT_ROOT/basics/basics-typescript | |||
| npm_install_check $PROJECT_ROOT/templates/typescript | |||
| npm_install_check $PROJECT_ROOT/templates/typescript-lambda-cdk | |||
| npm_install_check $PROJECT_ROOT/templates/cloudflare-worker | |||
| npm_install_check $PROJECT_ROOT/templates/bun | |||
| #npm_install_check $PROJECT_ROOT/templates/bun | |||
There was a problem hiding this comment.
I'll re-enable this in a follow-up PR, once I add a suitable bun-specific test.
| @@ -1,5 +1,7 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| set -eufx -o pipefail | |||
There was a problem hiding this comment.
set -e is the crucial part - without it intermediate tests can be failing arbitrarily, and won't cause the overall workflow to fail!
There was a problem hiding this comment.
this also makes remembering to the explicitly || exit all the tests redundant.
| @@ -12,7 +14,7 @@ npm_install_check $PROJECT_ROOT/basics/basics-typescript | |||
| npm_install_check $PROJECT_ROOT/templates/typescript | |||
| npm_install_check $PROJECT_ROOT/templates/typescript-lambda-cdk | |||
| npm_install_check $PROJECT_ROOT/templates/cloudflare-worker | |||
| npm_install_check $PROJECT_ROOT/templates/bun | |||
| #npm_install_check $PROJECT_ROOT/templates/bun | |||
There was a problem hiding this comment.
Does this need to be commented?
There was a problem hiding this comment.
it does because there's no "build" target there - fixed in #172 :-)
| deployer.deployService("Greeter", greeter.currentVersion, restateEnvironment, { | ||
| insecure: true, // self-signed certificate | ||
| // insecure: true, // accept self-signed certificate for SingleNodeRestateDeployment |
There was a problem hiding this comment.
Should we remove this if it's commented?
There was a problem hiding this comment.
I wanted to leave it in, in case someone uncomments the restate. restate.SingleNodeRestateDeployment(...) construct above which needs this. If we remove it, someone might lose a bunch of time troubleshooting why this fails with the self-hosted EC2 option. Open to removing both if you think it's cleaner?
This really should go away in the long run but I didn't want to open a non-TLS listener over the internet in the restate.SingleNodeRestateDeployment construct.
| @@ -75,9 +62,10 @@ export class LambdaTsCdkStack extends cdk.Stack { | |||
| }); | |||
|
|
|||
| deployer.deployService("Greeter", greeter.currentVersion, restateEnvironment, { | |||
| insecure: true, // self-signed certificate | |||
| // insecure: true, // accept self-signed certificate for SingleNodeRestateDeployment | |||
There was a problem hiding this comment.
Should we remove this if it's commented out?
With this PR, we're making sure that any test failing causes the entire GitHub workflow to fail fast. Previously, intermediate build failures were not causing the action to report an error.
Secondly, the CDK examples are updated so that they can build on GH without AWS creds; this also serves to make the more general and leaves it up to the user to decide whether to deploy to EC2 (commented out) or target an existing Restate server (either externally managed, or a Restate Cloud environment).
Ideally we should be testing the bun/deno/cloudflare templates with their respective deployment tools but that's outside of the scope of the current PR. I'll submit a separate change to include those in the test checks.