Skip to content

Adds git push template for python webapp2 with google_app_engine sdk#48

Open
jin09 wants to merge 6 commits into
k8s-platform-hub:masterfrom
jin09:master
Open

Adds git push template for python webapp2 with google_app_engine sdk#48
jin09 wants to merge 6 commits into
k8s-platform-hub:masterfrom
jin09:master

Conversation

@jin09

@jin09 jin09 commented Jul 18, 2017

Copy link
Copy Markdown

Added git push template for python webapp2. Tested on hasura local dev setup

@coco98

coco98 commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

@jin09 Thanks for the PR!

I've never used the jetbrains python IDE before, but I don't think we should be committing the .idea directory. If you're sure about this can you point me to a reference that says it's fine?
Here's a reference: https://stackoverflow.com/questions/11968531/what-to-gitignore-from-the-idea-folder

Also, can you include a .gitignore file?

I'll test it out in the meanwhile :)

@jin09

jin09 commented Jul 18, 2017

Copy link
Copy Markdown
Author

I have now included the .gitignore file and removed .idea folder which was unnecessary

@jin09

jin09 commented Jul 26, 2017

Copy link
Copy Markdown
Author

Hi,
Did you test it?

@ecthiender ecthiender left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey,

Sorry for the delay in reviewing this. I have added some comments, can you check those out?

Comment thread python-webapp2/Dockerfile Outdated

RUN pip install -r /home/src/requirements.txt

CMD ["python", "/home/google_appengine/dev_appserver.py", "/home/src/app.yaml", "--skip_sdk_update_check=yes", "--host", "0.0.0.0", "--port", "8080"] No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create an app directory instead of putting the source code in /home. That way it will be cleaner.


RUN pip install -r /home/src/requirements.txt

CMD ["python", "/home/src/main.py"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with having an app directory instead of putting it in /home.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, in the directory name of vanilla webapp2 use hyphens instead of underscore (as every other folder is hyphenated). So python-webapp2-vanilla instead of python_webapp2_vanilla.

@@ -0,0 +1,16 @@
import webapp2

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements.txt is empty. Are you sure it doesn't need webapp2?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't need webapp2 since it is already installed in the custom base Image that I have built for this (jin09/webapp2)

@jin09

jin09 commented Aug 22, 2017

Copy link
Copy Markdown
Author

I have made a few changes, please review them.

@coco98

coco98 commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

@jin09 Sorry for being totally AWOL on this.
I tried this out a while back and forgot to comment. I had a question:
What is the local development workflow for this? Can you add instructions to the README? (Eg: pip install -r requirements.txt followed by python main.py? By local development I mean that I want to test my python app locally before deploying it.

@jin09

jin09 commented Aug 23, 2017

Copy link
Copy Markdown
Author

@coco98 Workflow for local develoment is in the Dockerfile recipe itself. Or is there anything specific that you want me to add?

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