Add security api and fake client #44

redixin · 2016-12-05T19:07:15Z

No description provided.

coveralls · 2016-12-05T19:09:23Z

Coverage decreased (-0.02%) to 96.438% when pulling ed4470f on add-security-raml-and-fake-impl into 89c8e43 on master.

maretskiy · 2016-12-06T09:49:59Z

ceagle/api/v1/security.py


 security = flask.Blueprint("security", __name__)

+if base_fake.USE_FAKE_DATA:


This is yet another approach of Fake API implementation, there is another one already via decorator, example:

ceagle/ceagle/api/v1/status.py

Line 27 in 89c8e43

@fake_status.get_status

It is very doubtful that having two approaches is good choice, only one should be in use

maretskiy · 2016-12-06T09:55:14Z

raml/api.raml

+            region:
+              type: string
+              description: filter by region
+


It would be very helpful to add a reference to response example (like at L29-34)

coveralls · 2016-12-07T19:51:53Z

Coverage decreased (-0.7%) to 94.5% when pulling 9505907 on add-security-raml-and-fake-impl into a9c7499 on master.

coveralls · 2016-12-08T13:57:58Z

Coverage increased (+0.3%) to 95.455% when pulling d99f6dd on add-security-raml-and-fake-impl into a9c7499 on master.

coveralls · 2016-12-09T13:32:45Z

Coverage increased (+0.3%) to 95.455% when pulling fd94c67 on add-security-raml-and-fake-impl into a9c7499 on master.

Cesarsar · 2016-12-12T07:25:01Z

raml/api.raml

@@ -55,6 +55,18 @@ mediaType: application/json
              schema: !include schemas/404.json
              example: !include response_examples/404/region.json

+    /security/issues/{period}:


Will you introduce /security/stats/{period} API endpoint in this pull request? Or this will be doe separately?

coveralls · 2016-12-12T12:53:25Z

Coverage increased (+1.6%) to 96.86% when pulling d938a4f on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-13T14:21:02Z

Coverage increased (+1.3%) to 96.618% when pulling 0eee879 on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-13T14:36:50Z

Coverage increased (+1.3%) to 96.618% when pulling 101d0eb on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-13T20:23:00Z

Coverage increased (+1.3%) to 96.618% when pulling 1b59d5e on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-13T20:44:43Z

Coverage increased (+1.6%) to 96.852% when pulling 745e92c on add-security-raml-and-fake-impl into b81830c on master.

teferi

lgtm

coveralls · 2016-12-16T12:55:36Z

Coverage increased (+1.3%) to 96.61% when pulling 3896521 on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-16T13:08:34Z

Coverage increased (+1.3%) to 96.61% when pulling 7a50ebe on add-security-raml-and-fake-impl into b81830c on master.

maretskiy · 2016-12-16T15:00:47Z

ceagle/api/client.py

+    if config.get_config().get("use_fake_api_data", True):
+        fake_client_class = FAKE_CLIENT_MAP.get(service_name)
+        if fake_client_class:
+            return fake_client_class(name=service_name, endpoint="_fake_")


So this will return a fake client even if service_name is absent in config? Thi slooks like ignoring config options. I think both real & fake client should be returned only if service appears in config - this is checked at L74-76

maretskiy · 2016-12-16T15:02:18Z

ceagle/api/v1/security.py

@@ -14,15 +14,20 @@
 #    under the License.

 import flask
+from flask import request  # noqa


Why? flask.request is not a module! Use flask.request instead, without "# noqa"

Travis is failing on this line. Looks like a bug in some versions of flake8

pep8 must fail on this line since `request' is not a module

+1 you seem to never use request, so it's safe to just delete the line

maretskiy · 2016-12-16T15:09:00Z

ceagle/api/v1/security.py


 security = flask.Blueprint("security", __name__)

+client = client.get_client("security")


So if security service is missed in config file, this module will raise UnknownService at import-time (will not be imported at all, so main.py will failed to start) ?

Does this mean that security service is mandatory?

maretskiy · 2016-12-16T15:16:39Z

ceagle/api_fake_data/fake_security.py

+}
+
+
+class Client(object):


If inherited from ceagle.api.client.Client, there will be only one method get() overriden, but __ init __ and __ repr __ will work as usually

Done, but I had to move Client in separate module because of circular imports.

maretskiy · 2016-12-16T15:19:22Z

ceagle/api_fake_data/fake_security.py

+                if issue["discovered_at"] >= discovered_before:
+                    issues.append(issue)
+        else:
+            return "Region not found", 404


Is this ok to return str message instead of {"error": "Region not found"} ?

coveralls · 2016-12-19T12:42:24Z

Coverage increased (+0.9%) to 96.163% when pulling a555c81 on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-19T20:06:01Z

Coverage increased (+0.6%) to 95.923% when pulling e56aa92 on add-security-raml-and-fake-impl into b81830c on master.

coveralls · 2016-12-19T20:54:39Z

Coverage increased (+0.9%) to 96.163% when pulling 1312504 on add-security-raml-and-fake-impl into b81830c on master.

teferi · 2016-12-20T14:17:53Z

ceagle/api/v1/security.py

@@ -14,15 +14,20 @@
 #    under the License.

 import flask
+from flask import request  # noqa


+1 you seem to never use request, so it's safe to just delete the line

teferi · 2016-12-20T14:22:37Z

ceagle/main.py

@@ -44,6 +45,11 @@ def not_found(error):
    return flask.jsonify({"error": "Not Found"}), 404


+@app.errorhandler(client.UnknownService)
+def handle_unknown_service(ex):
+    return flask.jsonify({"error": str(ex)}), 400


I thing 404 is a bit more appropriate. It basically means all health/security/availability/whatever is not configured and all respective urls are unavailable. While 400 is usually viewed as some sort of problem with request itself https://httpstatuses.com/400

404 is "resource not found" but resource is found but misconfigured. This looks more like 500

coveralls · 2016-12-20T16:16:34Z

Coverage increased (+0.6%) to 95.913% when pulling f707005 on add-security-raml-and-fake-impl into b81830c on master.

fixed

coveralls · 2016-12-23T15:01:29Z

Coverage increased (+0.6%) to 95.913% when pulling 867b56d on add-security-raml-and-fake-impl into e0ba62c on master.

maretskiy suggested changes Dec 6, 2016

View reviewed changes

redixin force-pushed the add-security-raml-and-fake-impl branch from ed4470f to 9505907 Compare December 7, 2016 19:49

redixin force-pushed the add-security-raml-and-fake-impl branch from 9505907 to d99f6dd Compare December 8, 2016 13:55

redixin force-pushed the add-security-raml-and-fake-impl branch from d99f6dd to fd94c67 Compare December 9, 2016 13:30

Cesarsar reviewed Dec 12, 2016

View reviewed changes

Handle client misconfigurations by flask handler

84d13c3

redixin force-pushed the add-security-raml-and-fake-impl branch from fd94c67 to d938a4f Compare December 12, 2016 12:47

redixin force-pushed the add-security-raml-and-fake-impl branch from d938a4f to 0eee879 Compare December 13, 2016 14:14

redixin force-pushed the add-security-raml-and-fake-impl branch from 0eee879 to 101d0eb Compare December 13, 2016 14:29

redixin force-pushed the add-security-raml-and-fake-impl branch from 101d0eb to 1b59d5e Compare December 13, 2016 20:17

redixin force-pushed the add-security-raml-and-fake-impl branch from 1b59d5e to 745e92c Compare December 13, 2016 20:38

teferi approved these changes Dec 14, 2016

View reviewed changes

redixin force-pushed the add-security-raml-and-fake-impl branch from 745e92c to 3896521 Compare December 16, 2016 12:49

redixin force-pushed the add-security-raml-and-fake-impl branch from 3896521 to 7a50ebe Compare December 16, 2016 13:02

maretskiy suggested changes Dec 16, 2016

View reviewed changes

maretskiy previously requested changes Dec 16, 2016

View reviewed changes

redixin force-pushed the add-security-raml-and-fake-impl branch from 7a50ebe to a555c81 Compare December 19, 2016 12:35

redixin force-pushed the add-security-raml-and-fake-impl branch from a555c81 to e56aa92 Compare December 19, 2016 20:00

redixin force-pushed the add-security-raml-and-fake-impl branch from e56aa92 to 1312504 Compare December 19, 2016 20:49

teferi requested changes Dec 20, 2016

View reviewed changes

redixin force-pushed the add-security-raml-and-fake-impl branch from 1312504 to 14cfb7a Compare December 20, 2016 15:05

Add security service and fake data

f707005

redixin force-pushed the add-security-raml-and-fake-impl branch from 14cfb7a to f707005 Compare December 20, 2016 16:10

teferi approved these changes Dec 21, 2016

View reviewed changes

Merge branch 'master' into add-security-raml-and-fake-impl

867b56d

maretskiy approved these changes Dec 23, 2016

View reviewed changes

maretskiy merged commit d563eb4 into master Dec 23, 2016

boris-42 deleted the add-security-raml-and-fake-impl branch December 28, 2016 02:47


		security = flask.Blueprint("security", __name__)

		if base_fake.USE_FAKE_DATA:


		security = flask.Blueprint("security", __name__)

		client = client.get_client("security")

		}


		class Client(object):

Add security api and fake client #44

Add security api and fake client #44

Conversation

redixin commented Dec 5, 2016

coveralls commented Dec 5, 2016

maretskiy Dec 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 7, 2016

coveralls commented Dec 8, 2016

coveralls commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 12, 2016

coveralls commented Dec 13, 2016

coveralls commented Dec 13, 2016

coveralls commented Dec 13, 2016

coveralls commented Dec 13, 2016

teferi left a comment

Choose a reason for hiding this comment

coveralls commented Dec 16, 2016

coveralls commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maretskiy Dec 16, 2016 • edited

Choose a reason for hiding this comment

redixin Dec 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 19, 2016

coveralls commented Dec 19, 2016

coveralls commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 20, 2016

coveralls commented Dec 23, 2016

maretskiy Dec 6, 2016 •

edited

maretskiy Dec 16, 2016 •

edited

redixin Dec 19, 2016 •

edited