implement tgtm suggestions to apigw custom domain sample #246
Conversation
apigw-custom-domain/README.md
Outdated
| Serverless Domain Manager: Domain Name: test.example.com | ||
| Serverless Domain Manager: Target Domain: test.example.com | ||
| Serverless Domain Manager: Hosted Zone Id: Z2FDTNDATAQYW2 | ||
| ./run.sh |
There was a problem hiding this comment.
is there a reason to invoke this script directly, instead of using the make run command?
There was a problem hiding this comment.
make run invokes the smoke test as well with it.
There was a problem hiding this comment.
I envision that for local testing, we should rely more on shell scripts and use make targets for CI testing
There was a problem hiding this comment.
I have the same comment as @bartsdev here.
For consistency and discoverability, I think there is value in sticking to make as the single entry point. For example, users get confused if they leverage make usage and don't see a run target (which seems like one of the most important ones). I am always looking out for a Makefile or write one. It helps me tremendously when dealing with so many different repositories, languages, and frameworks.
There was a problem hiding this comment.
Great initiative 🚀
My main comments are regarding:
⚠️ Interpreting the exit code of bash script correctly when adopting them for CI- Migrating away from using make as entrypoint, but just for the run command
- Supporting configuration options, such as regions, properly if we provide them as parameter
apigw-custom-domain/README.md
Outdated
|
|
||
| ## Prerequisites | ||
|
|
||
| * LocalStack | ||
| * [`localstack` CLI](https://docs.localstack.cloud/getting-started/installation/#localstack-cli) with [`LOCALSTACK_AUTH_TOKEN`](https://docs.localstack.cloud/getting-started/auth-token/) |
There was a problem hiding this comment.
Generally important to clarify Pro vs. non-Pro image 👍
There was a problem hiding this comment.
Given @robertlcx comment — The prerequisites that are required for every sample would be moved to the root README while project-level READMEs would document any other prerequisite the particular sample needs.
There was a problem hiding this comment.
Valid point @robertlcx 👍 In general, it's often a balance between easier maintenance/consistency while being self-contained enough. I think we need to add an explicit link to the parent README, otherwise the dependency list could be misleading here.
apigw-custom-domain/README.md
Outdated
| Serverless Domain Manager: Domain Name: test.example.com | ||
| Serverless Domain Manager: Target Domain: test.example.com | ||
| Serverless Domain Manager: Hosted Zone Id: Z2FDTNDATAQYW2 | ||
| ./run.sh |
There was a problem hiding this comment.
I have the same comment as @bartsdev here.
For consistency and discoverability, I think there is value in sticking to make as the single entry point. For example, users get confused if they leverage make usage and don't see a run target (which seems like one of the most important ones). I am always looking out for a Makefile or write one. It helps me tremendously when dealing with so many different repositories, languages, and frameworks.
| @@ -0,0 +1,39 @@ | |||
| #!/bin/bash | |||
|
|
|||
There was a problem hiding this comment.
Bash scripts are hard to get right.
echo "done" at the end, the CI never fails even if all other commands fail ⚡ .
That oneliner can help set -euo pipefail (if we want to keep it short and strict).
See "Writing robust scripts and debugging" in Bash best practices
👉 I recommend running shellcheck when using Bash script
There was a problem hiding this comment.
All good points brought up by @joe4dev. I really like shellcheck too. Good practices.
There was a problem hiding this comment.
@joe4dev , do we have a bash script standard/best practices documented in our engineering docs? I'd even say that this is something we could add as pre-commit -> https://github.com/koalaman/shellcheck-precommit
There was a problem hiding this comment.
I don't think we have a documented standard for Bash.
Pre-commit hook + CI linting would be ideal moving forward 👍
|
|
||
| if [[ "$1" == "--ci" ]]; then | ||
| invoke_endpoints | ||
| else |
There was a problem hiding this comment.
💡 Make supports such dry runs out of the box: https://unix.stackexchange.com/a/275843
‘-n’
‘--just-print’
‘--dry-run’
‘--recon’
apigw-custom-domain/README.md
Outdated
|
|
||
| ## Prerequisites | ||
|
|
||
| * LocalStack | ||
| * [`localstack` CLI](https://docs.localstack.cloud/getting-started/installation/#localstack-cli) with [`LOCALSTACK_AUTH_TOKEN`](https://docs.localstack.cloud/getting-started/auth-token/) |
There was a problem hiding this comment.
Valid point @robertlcx 👍 In general, it's often a balance between easier maintenance/consistency while being self-contained enough. I think we need to add an explicit link to the parent README, otherwise the dependency list could be misleading here.
There was a problem hiding this comment.
These are great improvements @HarshCasper. The user experience is so much better 👏👏👏 🚀
The run command is currently broken in non-CI mode and should be fixed, ideally by using the same code path to ensure it's tested. Otherwise, it's looking good.
I replied to some other comments, but they are not blocking. I think having an explicit "force pro" flag upon LocalStack start would be very useful.
This PR:
prechecktarget to the Makefile to ensure prerequisites are available.run.shscript with a--ciflag for smoke tests.Overall, this PR serves as a testbed to reach an agreement with TGTM, Support, and Engineering on establishing standard consistency across the sample. Please provide suggestions or thoughts to improve this further! :)