Conversation
6b22c4e to
80f5edd
Compare
|
You are amazing! I will review the PR over the next weekend. I hope that is OK with you. |
|
Of course! 😊 |
|
Hey Hugo, I apologize for not having time to review your PR before. I have many obligations and do not have as much time as I used to. Plus it was re:Invent. I reviewed your code, and you did an absolutely amazing job. Unfortunately, some of the tests fail. For example: That is strange. Why would it need to bundle the Python code since it is Node.js only test. I presume that when the test builds the CDK to make a snapshot, it bundles the Python extensions. But that should be bundled when ServerlessSpy is built, not when ServerlessSpy is used in a new stack. Could you look at that issue? I granted you permission on the repository so you can merge the PR yourself when you are ready. Before you do:
|
|
No problem at alla Marko! Hmm that's odd, it passed when I ran the tests before submitting the PR, but I'll have a look and see what's wrong. And yeah I agree it shouldn't include any python code of there is no python lambda included in the tests. Haha yeah I promise I'll not remove you 😂 |
|
For the diagram, I used https://app.diagrams.net/, which is also available as a VsCode extension: The file is This is an amazing tool. Although the file is |
|
@ServerlessLife Would you mind sharing some more of the error logs you get when running the tests? I can't seem to reproduce it on my machine, all tests pass. Is it possible this is docker-related? The python bundling in cdk requires docker. I did however find why that specific test suddenly ran python-code. The test itself was referencing the SnsToLambdaStack instead of the SnsToSqsStack that it was testing. |
|
Also, I can't see any Author section in the README.md? Is it in some other file? Or should I add it? |
|
Here is full error log |
|
Regarding the author section. Currently, there is no such section. There is just my name in the header. You can make an author chapter at the end, move my name, and add yours. Or you can do something else. |
Hmm, that seems like a docker issue since it seems to fail when trying to write the asset output from the docker container. Are you running docker as another user or similar? |
|
It is a Docker issue, but there should not be any Docket at that point. Looks like CDK is using Docker to bundle Python code, but it should not need to bundle it at all. I am using Ubuntu. That could be a reason for a different output that you might have. |
80f5edd to
ca20f2a
Compare
|
I'm on Ubuntu too, so it shouldn't be just that. The snsToSqs test shouldn't need it and only called it because the snapshot test isn't building SnsToSqsStack, but SnsToLambdaStack, which is now fixed in my branch. The SnsToLambdaStack does deploy a Python lambda to test that the interceptor works. I can see that there is another issue with docker in the github runner. I'll see if I can figure out what is needed to get that to work. |
5c44f93 to
729f173
Compare
|
@ServerlessLife Okay I encountered some issues because of the jsii container that was used. It has since been removed as default in projen aws-cdk setup (projen/projen@978a4ea#diff-2304ffbbb1cb4987906a47b08b7c3b590fc542f4247b733844049776fd8591f2). |
5e76a4e to
3f00c0c
Compare
3f00c0c to
28ba7d1
Compare
|
Hey Hugo, I just found out the following 😞 :
So, in fact, you do not have access to secrets. I will look into a possible solution in the evening. I will also dive deeper into the Docker issue. |
Yeah I suspected something like that, and it kind of makes sense. I can run it in my fork and set up my own secrets if you want. Awesome 👍 |
|
It turns out the permissions work if the PR branch is from the local repo. So I merged your PR to a local branch and created a new PR and secrets works: #23 You can work on that PR or create a new one if you prefer. |
|
I figured out why the Docker gets involved when running the tests. The A similar problem is with TypeScript. If it is not compiled to JS, then it runs a Docker to compile ("transpile") if the user does not have Running a Docker when building a CDK is an issue because it is slow and can cause problems if the user does not have a Docker or has some configuration mismatches with it (apparently me). |
|
Yes docker is used to build the python layers. I'm not sure it's wise to prebundle the python layers (they are pretty sensitive to the environment being used to build them, especially awscrt breaks if the environment isn't correct). But I'll have a look. Could probably prebundle them with docker and use the output directly when building the layers. |
|
Closing as it has been handled in a local branch and PR to the project instead of from a fork. |
@ServerlessLife I've had to upgrade Typescript version to deal with a jsii issue introduced by aws-python-lambda. I also raised the cdk-lib version to get the proper Runtime class for Python 10 from aws-lambda.
I have not done anything to disable IoT when no clients are attached but as we spoke about it's probably not anything critical as the cost will be minimal anyway.
Let me know what you think of the implementation. I hope I have covered all of the tests that are relevant.