LOG-9289 apikeys and live tail supports #42

stopal-r7 · 2017-05-23T15:27:02Z

1st commit: Introduced apikeys and live tail supports to lecli
2nd commit: (No functional change here) There has been a major cleanup in UTs such that I have removed dummy urls and response examples that was not helping to test lecli functionality. Only left the ones that helps to verify lecli implementation
3rd commit: (Only functional change is around config file in this commit) I have tried to simplify and standardise url building logic among different api implementations. I have also removed management and query URLs from config file and introduced "api url" which is supposed to be the base url for REST API
4th commit: Owner support for listing api keys to show the owner api key itself and documentation for new APIs(live tail and api key management)

…ad of managemement/query url in config file

ellynch-r7 · 2017-05-23T15:30:00Z

lecli/api_key/api.py

+
+def create(payload):
+    """
+    Delete an api key with the provided ID


Think this was a copy-paste error!

ellynch-r7 · 2017-05-23T15:41:17Z

lecli/api_key/api.py

+            sys.stderr.write('Create api key failed, status code: %d' % response.status_code)
+            sys.exit(1)
+        elif response.status_code == 201:
+            sys.stdout.write('Created api key with payload: %s\n' % payload)


Are the response body & the payload here identical?

If not, we should probably print out the body.

we are printing the response body on the next line. I will remove printing the payload though, I think I put this in for debugging purposes. :)

eflanagan-r7 · 2017-05-23T15:57:33Z

lecli/api_key/api.py

+    Handle get api key response
+    """
+    if response_utils.response_error(response):
+        sys.exit(1)


Not printing out an error message here?

response_utils does

eflanagan-r7 · 2017-05-24T09:53:09Z

lecli/api_key/api.py

+    try:
+        response = requests.delete(url, headers=headers)
+        if response_utils.response_error(response):
+            sys.stderr.write('Deleting api key failed, status code: %d' % response.status_code)


How come we're printing this here? Does response_utils not print the status code?

nice catch, response_utils does print the status code so I will remove it from here.

eflanagan-r7 · 2017-05-24T09:55:37Z

lecli/api_key/api.py

+            sys.stderr.write('Deleting api key failed, status code: %d' % response.status_code)
+            sys.exit(1)
+        elif response.status_code == 204:
+            sys.stdout.write('Deleted api key with id: %s \n' % api_key_id)


What happens when this branch is false, and it's not a 204? If there's no possibility for another status code we can just have an else here and maybe a comment to say the status is 204

Same can be applied to all of the functions here

eflanagan-r7 · 2017-05-24T09:59:56Z

lecli/api_key/api.py

+        response = requests.get(url, headers=headers)
+        handle_api_key_response(response)
+    except requests.exceptions.RequestException as error:
+        sys.stderr.write(error)


These logging statements can be refactored to use the logging module built into python.
Might take a bit of time, so not a blocker.

Much nicer to have log.error(error) 🙂

This was done in this way for a reason -- possibly a feedback from a reviewer before. @ellynch-r7 do you remember why?

actualy a better way of doing this is with click.echo(err=True) as logging will require adopting logging for this as well + there are not really logs but information that has to be printed on screen.

I have used click.echo for team api, hopefully in the future we can use it all around the project. For the moment I have reached my refactoring limits :)

Does click.echo provide the option to add logging handlers? i.e. directing it to a file or a webhook? With python logging module we can just add le_python as a dependency and provide a log token

to a file yes not sure about logging handlers.

eflanagan-r7 · 2017-05-24T10:08:02Z

lecli/api_key/commands.py

+        with open(filename) as json_data:
+            try:
+                params = json.load(json_data)
+                print params


print statement here

eflanagan-r7 · 2017-05-24T10:09:56Z

lecli/api_key/commands.py

+                params = json.load(json_data)
+                print params
+                api.create(params)
+            except ValueError as error:


This should only be wrapped around the one line, params = json.load(json_data) right?
I don't believe api.create will throw that exception, or will it?

eflanagan-r7 · 2017-05-24T10:35:06Z

lecli/api_utils.py

+        else:
+            print_config_error_and_exit(URL_SECTION, 'Log Query URL(%s)' % config_key)
+    except (ConfigParser.NoOptionError, ConfigParser.NoSectionError):
+        return 'https://rest.logentries.com/query'


This should be extracted as a constant. So it's easier to modify later. As well as the other default urls in the file

eflanagan-r7 · 2017-05-24T10:36:30Z

lecli/cli.py

+@cli.group()
+def tail():
+    """Tail logs"""
+    pass


Should we bother committing this if it's incomplete?

this isn't incomplete actually, its there for registering that group with click context

eflanagan-r7 · 2017-05-24T10:39:24Z

lecli/query/api.py

+            next_url = response.json()['links'][0]['href']
+            time.sleep(poll_interval)
+            response = fetch_results(next_url)
+


What happens here if there is no links field in the response?

there has to be a link in the response as its live tail

Then should we print an error? It will just end silently if something weird comes back

eflanagan-r7 · 2017-05-24T10:49:28Z

tests/test_apikey_api.py

+    api.list()
+
+    out, err = capsys.readouterr()
+    assert not err


Can we check that something has been printed to out?
If someone creates a branch with no error output, it will still pass

eflanagan-r7 · 2017-05-24T10:52:11Z

tests/test_apikey_api.py

+
+    out, err = capsys.readouterr()
+    assert not err
+    assert 'Disabled api key with id: %s' % api_key_id in out


There's no guarantee from this that we've sent the correct body. Can we check what calls are made to the mocked http client?

yeah can check that i guess with httpretty.last_request 👍

…ed on POST requests

stopal-r7 added 3 commits May 22, 2017 17:05

LOG-9289 add live tail and apikey management supports and UTs

054af7c

LOG-9289 UT framework cleanup

516305e

LOG-9289 simplifying url building logic and introducing api url inste…

ef4f368

…ad of managemement/query url in config file

stopal-r7 requested review from eflanagan-r7 and ellynch-r7 May 23, 2017 15:27

ellynch-r7 reviewed May 23, 2017

View reviewed changes

eflanagan-r7 reviewed May 23, 2017

View reviewed changes

LOG-9289 owner support for listing api keys + documentation for new APIs

d8bd207

ellynch-r7 approved these changes May 23, 2017

View reviewed changes

eflanagan-r7 reviewed May 24, 2017

View reviewed changes

stopal-r7 added 2 commits May 24, 2017 14:54

LOG-9289 review fixes

f5e9503

LOG-9289 removing label support from lvie tail as its not yet support…

3d3efa4

…ed on POST requests

eflanagan-r7 approved these changes May 24, 2017

View reviewed changes

stopal-r7 force-pushed the LOG-9289 branch from eb0185b to 3d3efa4 Compare May 24, 2017 14:09

stopal-r7 merged commit 9d925e4 into rapid7:master May 24, 2017

stopal-r7 deleted the LOG-9289 branch May 30, 2017 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOG-9289 apikeys and live tail supports #42

LOG-9289 apikeys and live tail supports #42

stopal-r7 commented May 23, 2017 •

edited

Loading

ellynch-r7 May 23, 2017

ellynch-r7 May 23, 2017

stopal-r7 May 23, 2017

eflanagan-r7 May 23, 2017

stopal-r7 May 23, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017

stopal-r7 May 24, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017 •

edited

Loading

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017 •

edited

Loading

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017

eflanagan-r7 May 24, 2017

eflanagan-r7 May 24, 2017

stopal-r7 May 24, 2017

LOG-9289 apikeys and live tail supports #42

LOG-9289 apikeys and live tail supports #42

Conversation

stopal-r7 commented May 23, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

stopal-r7 May 24, 2017 • edited Loading

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

stopal-r7 May 24, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stopal-r7 commented May 23, 2017 •

edited

Loading

stopal-r7 May 24, 2017 •

edited

Loading

stopal-r7 May 24, 2017 •

edited

Loading