8000 Api crud by teferi · Pull Request #9 · seecloud/runbook · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Api crud #9

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

Closed
wants to merge 2 commits into from
Closed

Api crud #9

wants to merge 2 commits into from

Conversation

teferi
Copy link
Collaborator
@teferi teferi commented Dec 16, 2016

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+73.2%) to 73.248% when pulling b44ce4f on api_crud into 926a517 on storage.

from runbook import config


def check_regions(func):
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should move this over time to oss-lib

# should not really be here
return flask.jsonify({"error": "Was unable to save document"}), 500
else:
result = es.search(index="ms_runbooks_region_one", doc_type="runbook")
Copy link
Member

Choose a reason for hiding this comment

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

so I don't understand why index is hardcoded here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

elif flask.request.method == "DELETE":
try:
es.delete(
index="ms_runbooks_region_one",
Copy link
Member

Choose a reason for hiding this comment

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

name should not be hardcoded

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

except elasticsearch.NotFoundError:
flask.abort(404)

elif flask.request.method == "PUT":
Copy link
Member

Choose a reason for hiding this comment

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

probably it makes sense to move code related PUT, POST, GET to helpers so it will be way simpler to write unittests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can use smth like flask-restfull if this becomes a problem. However since we're splitting API into ro/wo the functions would get simpler.

http://flask-restful-cn.readthedocs.io/en/0.3.5/

"regions": [
"region_one",
"region_two"
],
Copy link
Member

Choose a reason for hiding this comment

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

please remove ","

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

self.addCleanup(mock.patch.stopall)

# NOTE(kzaitsev): mock all get_config for a ll the tests
self.patcher = mock.patch('runbook.config.get_config')
Copy link
Member

Choose a reason for hiding this comment

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

can we mock config in place othewise it's really hard for newbies to understand where it is mocked?

self.get_config = self.patcher.start()
self.get_config.return_value = TEST_CONFIG

# NOTE(kzaitsev): importing this module calls get_config, so I'm
Copy link
Member

Choose a reason for hiding this comment

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

Module should not import config.
We should not have any long running operations (like reading files) during the module imports

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not the case for all our projects currently — main.py calls get_config at module level. should be trivial to change that though

It is now possible to create, update and delete runbooks in
elasticsearch. Input validation is done with jsonschema.
Also fix result conversion, from always returning the same runbook
@coveralls
Copy link

Coverage Status

Coverage increased (+73.6%) to 73.585% when pulling b380265 on api_crud into 926a517 on storage.

@teferi
Copy link
Collaborator Author
teferi commented Dec 19, 2016

Closing in favor of #12 and #13

@teferi teferi closed this Dec 19, 2016
@teferi teferi deleted the api_crud branch January 10, 2017 15:37
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.

3 participants
0