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

Reload Flask on API file changes #1418

Merged
merged 6 commits into from
Feb 1, 2022
Merged

Conversation

aparcar
Copy link
Contributor

@aparcar aparcar commented Aug 25, 2021

Flask can automatically reload in debug mode whenver specified files
change. By default that includes templates, however changes to the API
files (i.e. openapi.yml) should result in a reload as well.

This PR adds all added API files to be monitored automatically by Flask.

WIP since I have to figure out the used tox setup first.

Signed-off-by: Paul Spooren mail@aparcar.org

@Ruwann
Copy link
Member

Ruwann commented Sep 1, 2021

Hi @aparcar , thanks for the PR, let me know if you need some help

@aparcar
Copy link
Contributor Author

aparcar commented Sep 1, 2021

@Ruwann Thanks for your response. I'll look into adding test cases to this PR.

I'm not sure why the current CI is failing on Python 3.9, do you have any ideas?

Also do you require any doc updates for this PR?

@@ -101,7 +103,8 @@ def run(self, port=None, server=None, debug=None, host=None, **options): # prag

logger.debug('Starting %s HTTP server..', self.server, extra=vars(self))
if self.server == 'flask':
self.app.run(self.host, port=self.port, debug=self.debug, **options)
self.app.run(self.host, port=self.port, debug=self.debug,
extra_files=self.extra_files, **options)
Copy link
Member

Choose a reason for hiding this comment

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

The checks are failing because this line is not correctly indented. It should be aligned with the self.host in the line above (add 5 spaces I think).

@coveralls
Copy link

coveralls commented Sep 2, 2021

Coverage Status

Coverage increased (+0.02%) to 97.053% when pulling 13ebc66 on aparcar:extra_files into 2dfd57d on zalando:main.

@@ -64,6 +65,7 @@ def common_error_handler(self, exception):
def add_api(self, specification, **kwargs):
api = super().add_api(specification, **kwargs)
self.app.register_blueprint(api.blueprint)
self.extra_files.append(specification)
Copy link
Member

@Ruwann Ruwann Sep 2, 2021

Choose a reason for hiding this comment

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

This can be more than just str, it can also be a pathlib.Path or dict instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it will only be added in case it's either string or Path

@Ruwann
Copy link
Member

Ruwann commented Sep 2, 2021

For the test cases, I'm not sure whether it is possible to write a test to check whether it is reloading in debug mode? You could write a test for checking whether they are added to the extra_files, but not sure how useful that test is.

I don't think we necessarily have to add documentation for it, for me it feels natural that the app is reloaded when the swagger is changed. But if you find a good place to add it, feel free to make a suggestion.

Flask can automatically reload in debug mode whenver specified files
change. By default that includes templates, however changes to the API
files (i.e. openapi.yml) should result in a reload as well.

This PR adds all added API files to be monitored automatically by Flask.

Signed-off-by: Paul Spooren <mail@aparcar.org>
Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

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

We also need to add this option now to FlaskApp.run() because users might already use the extra_files argument for other things. So we need to append the extra_files in the run() function to the self.extra_files.
Otherwise, this would lead to a TypeError: run() got multiple values for keyword argument 'extra_files'

Is it clear what I mean?

@@ -64,6 +65,8 @@ def common_error_handler(self, exception):
def add_api(self, specification, **kwargs):
api = super().add_api(specification, **kwargs)
self.app.register_blueprint(api.blueprint)
if isinstance(specification, (str, pathlib.Path)):
self.extra_files.append(specification)
Copy link
Member

Choose a reason for hiding this comment

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

We need to take into account the specification_dir option (see for example the openapi3/helloworld example).

Suggested change
self.extra_files.append(specification)
self.extra_files.append(self.specification_dir / specification)

User can still specify custom extra_files in addition to the
swagger specification, so need to account for that in FlaskApp.run().
@aparcar aparcar marked this pull request as ready for review January 30, 2022 19:17
@Ruwann
Copy link
Member

Ruwann commented Feb 1, 2022

@RobbeSneyders This PR looks good to me, but I have made some changes myself as well, let me know if you'd like to have another look at it before it is merged.

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thx @aparcar @Ruwann.
One small nit, but LGTM.

connexion/apps/flask_app.py Show resolved Hide resolved
@Ruwann Ruwann merged commit 6abcbf6 into spec-first:main Feb 1, 2022
vbxx3 pushed a commit to Savannah-Group/connexion that referenced this pull request Apr 9, 2022
* Reload Flask on API file changes

Flask can automatically reload in debug mode whenver specified files
change. By default that includes templates, however changes to the API
files (i.e. openapi.yml) should result in a reload as well.

This PR adds all added API files to be monitored automatically by Flask.

Signed-off-by: Paul Spooren <mail@aparcar.org>

* Take into account specification dir

* Add extra_files argument.

User can still specify custom extra_files in addition to the
swagger specification, so need to account for that in FlaskApp.run().

* Update docstring for flake8

* Add extra_files as explicit argument for FlaskApp

* Add extra_files docstring

Co-authored-by: Ruwan <ruwanlambrichts@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants