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

WSGI for st2auth and st2bundle #100

Merged
merged 8 commits into from
Jan 25, 2016
Merged

WSGI for st2auth and st2bundle #100

merged 8 commits into from
Jan 25, 2016

Conversation

arm4b
Copy link
Member

@arm4b arm4b commented Jan 22, 2016

Let's open PR, since almost all blockers resolved with no need of unix sockets anymore and we can commit, rather then experiment/try & force-push.

Addresses issue #94

Most of the work is done by @enykeev

enykeev and others added 8 commits January 25, 2016 15:32
 Since it's not requirement anymore for unix sockets to be saved in, but part of another still undecided issue: #99
Found during another session: 650b164
Currently, st2api requires +w on packs to be able to read pack icon. Not sure why it's necessary, but to unblock me, I'm going to give it. Please don't remove until the problem is solved in st2.
@enykeev
Copy link
Member

enykeev commented Jan 25, 2016

@armab I'd say we're ready to merge. If it looks fine to you, merge it and I'm going to test it across all the platforms from bintray tomorrow.

@arm4b
Copy link
Member Author

arm4b commented Jan 25, 2016

Looks nice ;)

Let me debug a little (CircleCI + SSH).

If timeout increase worked (stable), then we should better hardcode it in st2-packages internal files, without touching circle.yml.

To avoid touching circle.yml in st2 repo.

@arm4b
Copy link
Member Author

arm4b commented Jan 25, 2016

Regarding failed build: https://circleci.com/gh/StackStorm/st2-packages/262

I could run tests with ST2_WAITFORSTART=1 for 10 times in a row in CircleCI + SSH environment, so couldn't reproduce it.

docker-compose -f docker-compose.circle.yml run -e ST2_GITURL=https://github.com/StackStorm/st2 -e ST2_GITREV=master -e ST2PKG_VERSION=1.4dev -e ST2PKG_RELEASE=8 -e RABBITMQHOST=172.17.0.1 -e POSTGRESHOST=172.17.0.1 -e MONGODBHOST=172.17.0.1 -e ST2_WAITFORSTART=1 wheezy test

Let's merge, but if it appears once again - what we can do in future to debug better - share /var/log/st2 logs and service logs via Circle artifacts, to read these files once something wrong appears (could be a good idea in any way).

arm4b pushed a commit that referenced this pull request Jan 25, 2016
WSGI for st2auth and st2bundle
@arm4b arm4b merged commit 43725de into master Jan 25, 2016
@arm4b arm4b deleted the fix/inits branch January 25, 2016 15:07
@arm4b
Copy link
Member Author

arm4b commented Jan 25, 2016

Good to know, that even with 0 timeout it worked well:
https://circleci.com/gh/StackStorm/st2-packages/268

See this build: https://circleci.com/gh/StackStorm/st2-packages/268
where I accidentally set timeout to 0, described here: #105

DAEMON=/usr/share/python/st2api/bin/$NAME
DAEMON_ARGS="--config-file /etc/st2/st2.conf"
DAEMON=/usr/share/python/st2api/bin/gunicorn_pecan
DAEMON_ARGS="/usr/share/python/st2api/lib/python2.7/site-packages/st2api/gunicorn_config.py -k eventlet -b 127.0.0.1:9101 --threads 10 --workers 1"
Copy link
Contributor

Choose a reason for hiding this comment

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

@enykeev , @armab this is inappropriate, there's a config file which is meant to be customized. You make it static by hardcoding the path to the default same way it was before.
cc: @lakshmi-kannan

Good way is:

  1. make, say it, /etc/st2/config.
  2. place the file there

So the enduser can get control of it.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

gunicorn does not allow you to pass arguments the same way you're passing them to the script. You can although redefine config pach using env variable https://github.com/StackStorm/st2/blob/master/st2api/st2api/gunicorn_config.py#L35-L36. I don't see it as an immediate concern, the amount of users who need to change the config is extremely low, but if you believe this is a problem, feel free to pass the variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 @enykeev

In this special case like it more too, comparing to config file.
And path is already hardcoded in many other startup scripts, don't see any inconsistencies here.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep. that's right I agree. thanks for clarifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants