-
Notifications
You must be signed in to change notification settings - Fork 402
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
docs(homepage): Change installation to CDK v2 #4351
docs(homepage): Change installation to CDK v2 #4351
Conversation
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
Hey Keith thanks a lot for this quality improvement 👌 -- could you also open up a docs issue as we require for PRs? |
Thanks @heitorlessa. Done. |
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.
Hello @keithrozario, thank you for bringing this to our attention.
We have linter, type checking and others to try to keep our documentation as real as possible, the reason we don't get this error is because the code is inline in the md file. I'm thinking about moving this file to the examples
folder and including it in md, so that our CI can observe it. Could you do this please? If you don't have time now, I can do and approve/merge this PR today.
Thanks @leandrodamascena I can certainly do that -- but perhaps not today :). But I'm also thinking there's a trade-off. For example, this snippet doesn't include the Thoughts? |
Yes, really good points to worry about. But I think we have a solution: 1 – We can highlight lines to show what is most important in that example Wjhat do you think about? |
Makes sense. I'll work on that soon :) |
Signed-off-by: Keith Rozario <[email protected]>
I was performing the change today, I thought it would be best to move all code snippets to the examples folder. I thought this would make it more consistent, and we would be able to lint/check these files more accurately in the future. I also noticed the SAR example was using CDK v1, and upgraded that to CDK v2 as well. It does introduce a bunch of new example files -- but the index.md now looks a lots more cleaner and consistent just referencing those code blocks rather than including them in-line (in my view at least :)) |
Quality Gate passedIssues Measures |
Hey @keithrozario! Fantastic changes, and we're now able to identify inconsistencies more effectively since we're running linter/formatter on those files. I made some minor adjustments, and it's now ready for merging 🚀 |
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
Issue number: #4353
Summary
Updated the docs/homepage to reflect CDK v2 instead of CDK v1.
Changes
Changed the docs/homepage. Installation steps are now for CDK v2 instead CDK v1. The original docs had a CDK v1 implementation which would fail if the user used CDK v2 with an
ImportError
. Also tidied up the environment to useAws.Region
. This drops inRef: AWS::Region
in the cloudformation stack and should work across all regions.User experience
No user experience changes.
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
No. **RFC issue number**:Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.