Skip to content
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

CSharp and Java versions for the the-scalable-webhook stack. Update TS and Python Stack to CDK 1.82.0. Documentation issues. #153

Merged

Conversation

leandrodamascena
Copy link
Contributor

Good morning. In this PR I created the C# and Java version and made some changes in TS/Python stacks, basically in the code comments and CDK version.

1 - TS and Python versions updated to CDK 1.82.0.
2 - Assuming that you consider TS version as the "main" version, the ReservedConcurrentExecutions parameter for the subscribe function was missing in Python version.
3 - Fixed some comments. Just to keep the things organized.
4 - In the Python version I fixed a possible problem with fopen function. Assuming that we have different encodes in different computers, fopen function should contain the encode to UTF8.
5 - In the Java version I have more considerations:

  • 5.1 - I read the "the-simple-webservice" stack and it contains a wrapper to mvn. Ok, I use it for some systems, but I don't think it's necessary. Assuming that this repository should contain and follow the CDK guidelines, I think we should keep the default option (using default mvn) and it's ok.
  • 5.2 - As the Java CDK version there isn't cdk-assert, I added basic tests to Java. Then I think we can evolve from this point.

Let me know if I need to change anything.

Ty

image

@nideveloper
Copy link
Collaborator

for 4 that is actually boilerplate cdk python code, I didn't create that file myself so maybe something you should consider creating a PR for CDK itself for the python init template https://github.com/aws/aws-cdk/blob/2e067d7d00b10b4c5e26665386c0a86340a83379/packages/aws-cdk/lib/init-templates/v1/app/python/setup.template.py#L4

on 5 there was a big discussion about mvnw with Java, I said keep it as unopinionated as possible but I wanted that direction to be driven by real java cdk users. Driss did move the lambda functions to Java too in the simple webservice with tests so not sure if that added complexity. Either way, you can see all the other langs are boilerplate so i'm not married to it.

Speaking of tests for 5.2, i'm not sure it is worth including super basic tests. I don't add them to Python because everyone I ask who uses cdk in python doesn't do infra testing yet because the assert lib isnt available. I think the focus would be better spent pushing AWS to properly implement assert cross language through issues like this one aws/aws-cdk#9614 . This is the main reason I personally use TS CDK for everything real still today. It's not that I think the tests are bad or anything but string matching will only get you so far and this is kind of like mvnw in that it is an opinion as opposed to a standard everyone is doing

@leandrodamascena
Copy link
Contributor Author

4 - ok, makes sense openning a pr to change initial template instead change only cdkpatterns implementation. I'll revert this change

5.1 - I'm not sure if it is very important change the lambda function from ts to java/c# or other language. I think the main point of a pattern is how to build this using cdk, how it works and the resources that it uses. But on the other hand I think we can bring more value to community if this show beyond the only cdk stack and show how to deploy a lambda function using Java. Ok, let me rebuild this using Java, not a real problem do this.

about mnvm, i think it is not a problem to keep mvn as default, i use it in production and for me it is good, but let me know if i need to change it.

5.2 - Ok ok, string tests are very basic and do not bring any value. I will remove this as well and vote for the issue to add it in all languages.

This PR can be the basis for others that I opened, so I'll wait the decisions and replicate it to another one.

@nideveloper
Copy link
Collaborator

Sorry I don’t think you need to rewrite all the js functions as java for me to merge the pr I just wasnt sure if adding that extra part of the build in caused a complexity that mvnw solved. Tbh when I deployed it myself I already have mvn installed so I deployed it the old fashioned way

I think just the tests and it’s good to merge

@leandrodamascena
Copy link
Contributor Author

Aaa ok, got it now! I misunderstood your first explanation. I think that in the near future it's important to refactor all functions to the CDK language, so that a person who only knows Java (assuming he/she has no idea about js/python), can understand the whole pattern.

I will remove the test directory from all PR and update it.

btw, reading "Converting Lambda Fns to CDK Language" I found a typo: "Lambdba". I'll include in this PR.

@nideveloper
Copy link
Collaborator

last minor thing and then this is good to merge - you are missing the publish lambda function from the .net version

@leandrodamascena
Copy link
Contributor Author

ohh .gitignore csharp version! let me fix lol

@cdk-patterns cdk-patterns merged commit 0595a68 into cdk-patterns:master Jan 10, 2021
@leandrodamascena leandrodamascena deleted the feature/the-scalable-webhook branch January 11, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants