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

Add support for app.sh script so have easy control over how application launched. #174

Merged
merged 5 commits into from
Feb 6, 2017
Merged

Add support for app.sh script so have easy control over how application launched. #174

merged 5 commits into from
Feb 6, 2017

Conversation

GrahamDumpleton
Copy link
Contributor

This address issue #126.

Note that you should merge change #165 first as this depends on the test case added in that for uWSGI, which uses an app.sh file executed from an app.py file. When the change here is then merged, the app.sh file in the test will take precedence and the app.sh will be executed directly with app.py being bypassed.

Note that this change doesn't do anything about APP_HOME as existing code didn't do anything with APP_HOME for app.py either. Arguably it should, but this is being dealt with in separate issues and PR.

Also note that although the change uses APP_SCRIPT and refers to it as application script file, encouraging it as being used for a shell script, technically it could be any executable script file, or even a compiled application. A generic name for the environment variable could be used such as APP_PROGRAM if really want to reflect that, but probably better to encourage it being just used for a shell script which then in turn can execute anything.

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@rhscl-bot
Copy link

Can one of the admins verify this patch?

@pkubatrh
Copy link
Member

#165 has been merged, please rebase so that we can use the tests in CI

@pkubatrh
Copy link
Member

[test]

Copy link
Member

@pkubatrh pkubatrh left a comment

Choose a reason for hiding this comment

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

Just a minor issue in the README files, otherwise LGTM.

This is the most general way of executing your application. It will be
used in the case where you specify a path to an executable script file
via the `APP_SCRIPT` environment variable, defaulting to a file named
`app.sh` if it exists. The script is executed directory to launch your
Copy link
Member

Choose a reason for hiding this comment

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

I guess directory -> directly?

@pkubatrh
Copy link
Member

One more thing - this part should also get changed to accommodate for the new APP_SCRIPT variable
https://github.com/GrahamDumpleton/s2i-python-container/blob/7d09b2ba22c60a1c012fed00b5f11cc13375e7f6/2.7/s2i/bin/run#L102

@GrahamDumpleton
Copy link
Contributor Author

Added APP_SCRIPT environment variable to final error message.

@hhorak
Copy link
Member

hhorak commented Jan 26, 2017

LGTM.

@hhorak
Copy link
Member

hhorak commented Jan 26, 2017

[test]

@hhorak hhorak merged commit b73c524 into sclorg:master Feb 6, 2017
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.

None yet

5 participants