-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
from runbook import config | ||
|
||
|
||
def check_regions(func): |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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": |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
"regions": [ | ||
"region_one", | ||
"region_two" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove ","
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
No description provided.