-
Notifications
You must be signed in to change notification settings - Fork 271
add dev setup guide #656
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
add dev setup guide #656
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Jerad. Followed the Dev Setup guide, looks good to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 👍 Appreciate the clear descriptions and thorough steps.
export TERMINATOR_NAME=<name> | ||
export QUEUE_URL=$(./scripts/get-cfn-stack-output.sh "${QUEUE_STACK_NAME}" QueueURL) | ||
|
||
envsubst <resources/terminator.yaml.tmpl >terminator-${TERMINATOR_NAME}.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to previous section, I think it's better to state what needs to be done (substitution) and let user decide how to do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I view this as a how-to guide. When I'm reading a how-to guide I prefer clear and detailed instructions rather than the implementation being left as "an exercise for the reader".
Specifically referencing the use of envsubst
: below the code block, alternate instructions are given if the reader does not wish to use that tool.
charts/aws-node-termination-handler-2/templates/node.k8s.aws_terminators.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Issue #, if available:
Description of changes:
Re-organize files and add instructions for setting up a development environment (see DEVELOPMENT.md).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.