Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add python support to cdk init #2130
feat: Add python support to cdk init #2130
Changes from 2 commits
c6bfc0a
269f1fd
8f23fa7
713f4a2
406d846
23b413c
55c5639
46de258
9d30dcc
85ccb5b
f36ae06
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This template should actually be called "sample-app". The "app" template should result in an empty stack (so you don't have to go delete things). See typescript as an example.
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'm confused by this comment. I was following the existing examples for
csharp
,java
, etc. Is this inconsistent with them?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.
Well.... those should be changed to ;-)
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.
Ah, I see. Yeah, its kind of backwards now. Do you want to fix all of them now?
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.
So do you want to change all of those now?
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.
Let's start by renaming this specific template to
sample-app
and then follow up with a PR for an emptyapp
in python.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.
Mention python version used.
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 believe we don't have an app.sh script here (which is a good thing).
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.
Isn't
develop
for libraries (as opposed to apps)?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.
develop
is just a variant ofinstall
that allows you to edit your code and run it without having to re-install. This example really consists of both an app and a library. I have restructured the code slightly to make that more clear, I hope.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.
Could this include running via the venv? Isn't that a better experience than requiring users to have pre-actived their shell and THEN running
cdk synth
?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.
Yes, that's a good idea. We should be able to just point to the python3 executable in the virtualenv.
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.
The EXE on Windows (for 3.7 at least as I just installed it last week) is just python.
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.
This gets ugly. If we are using a virtualenv that has been created using Python3 then there is no real need to say
python3
here. However, that does mean that during the init process we have done apython3 -m venv .env
so this, too, will fail on Windows.We could try
python3
for the virtualenv and then trypython
if that fails but this would cause problems on a machine that does not have Python3 installed at all because we would end up with a virtualenv for Python2.Maybe this is an argument against trying to create the virtualenv during the init process.
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.
It is possible to actually have the app entrypoint an executable file that takes care of all these details, so eventually
cdk.json
would be: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.
Does it make sense in python-land to have this duplicated with requirements.txt?
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.
Many packages use both but I have simplified this to put the requirements in the
setup.py
and then tell pip to use those rather than list them explicitly inrequirements.txt
.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.
Would it make sense to also do the
pip install
stuff here so people can get started quicker?