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

Make app creating factory async #23

Merged
merged 2 commits into from Apr 1, 2018

Conversation

gyermolenko
Copy link
Contributor

@gyermolenko gyermolenko commented Mar 26, 2018

To be able to run application with gunicorn I made the application creating factory async (fix #10).
Unfortunately I got unrecognized arguments error from

gunicorn aiohttpdemo_polls.main:init_app --bind localhost:8080 --worker-class aiohttp.GunicornWebWorker

so I've also substituted parse_args with parse_known_args as a workaround:

# ignore unknown options
options, unknown = ap.parse_known_args(argv)

Please review get_config function usages and parts of logic related to usage of argv and get_config(['-c', USER_CONFIG_PATH.as_posix()]). It looks ugly but I can't find a better way (if we still want command line args).

@f6m6
Copy link

f6m6 commented Mar 26, 2018

As soon as the tests pass let's merge this!

@jettify
Copy link
Member

jettify commented Mar 26, 2018

what about other demos? any plan to make factories there?

@webknjaz
Copy link
Member

webknjaz commented Mar 28, 2018

It seems to me that you re-implement what's available in https://pypi.org/project/aio_manager/ (manage.py / argparse wrapper, might be overkill for now) + https://pypi.org/project/aio_yamlconfig/ (reads yaml config, checks it against trafaret schema, saves to app.config), is it your intention to avoid external dependencies?

@gyermolenko
Copy link
Contributor Author

@webknjaz
I don't avoid external dependencies. aio_manager looks like an overkill, but aio_yamlconfig can probably fit. Although I was thinking about yamllint. Need some opinions from others.

We have at least 3 kinds of app start:

  1. python main.py
  2. tests
  3. gunicorn

So if we want flexible app configuration - what are best practices here?
Current implementation is to pass -c ./path/to/config_file.xxx from command line for start option 1... and 2 workarounds (for start options 2 and 3).

@gyermolenko
Copy link
Contributor Author

@jettify

what about other demos? any plan to make factories there?

I can do that, sure. Do we want other similarities between different demos?

@webknjaz
Copy link
Member

I'd say priority for config lookup should follow unix-way:

  1. passed as CLI arg (-c, --config)

    1.1. (maybe) $PWD/config/app_config_name.yaml

  2. ~/.app_config_name.yaml

  3. /etc/app_config_name.yaml

@jettify
Copy link
Member

jettify commented Mar 31, 2018

@gyermolenko not sure but essential parts should be the same I think like factories, new APIs

@jettify
Copy link
Member

jettify commented Mar 31, 2018

@webknjaz would you like submit PR with your suggestion?

Copy link
Member

@jettify jettify left a comment

Choose a reason for hiding this comment

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

Feel free to merge. Other improvements could be done in separate PRs

@gyermolenko gyermolenko merged commit 0a59dc5 into master Apr 1, 2018
@webknjaz
Copy link
Member

webknjaz commented Apr 3, 2018

@jettify i'll do if i have time

@gyermolenko gyermolenko deleted the use_application_factory_to_create_app branch May 22, 2019 12:51
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.

Demonstrate gunicorn usage
4 participants