Conversation
RomainMuller
left a comment
There was a problem hiding this comment.
I stopped reporting at some point, but you'll want to make sure all your main functions start with defer jsii.Close().
If the cdk init template does not have this... we should probably fix that...
|
Addressed these comments here and in our templates aws/aws-cdk#21728 |
These changes were brought up in a [review of the Go workshop](aws-samples/aws-cdk-intro-workshop#636) - update Go version - close jsii process - remove unneeded `assertions` alias ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
RomainMuller
left a comment
There was a problem hiding this comment.
Made a bunch of styling adjustments but other than that this was good to go...
Some style fixes... Running `go fmt` and `goimports` to ensure code looks canonical. Changed branch names from `master` to `main` everywhere. Fixed a couple of missed edits, etc...⚠️ This is a PR on top of #636 --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
indrora
left a comment
There was a problem hiding this comment.
Quality of life things:
- Across the workshop (not required by this PR!) we should work on wording things a little clearer.
- I want alt-text. There's 36 spots of it.
Code wise I think there's no major nitpicks except the occasional placement of an interface reference in a struct declaration (which is ignored by the compiler, for various reasons).
I'm going to hold on alt-text though. Doesn't take much but we can port it around for other portions of the workshop.
| } | ||
|
|
||
| type cdkWorkshopStack struct { | ||
| awscdk.Stack |
There was a problem hiding this comment.
This isn't really the right place for this? I don't know if this is auto-generated or not but you don't typically place interface declarations inside a struct.
There was a problem hiding this comment.
This is actually what Romain suggested to me when I was struggling to figure out how to "extend" a "class". The way this works makes sense to me within my limited experience of the language, what would you suggest as an alternative?
Creates workshop for Go
Still need to add advanced topic sections for testing and pipelines
also fixes #275
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.