Issues 109 #132

BlackVegetable · 2017-09-13T15:31:39Z

Added support for access keys. Do we need to bump the version number before merging this?

arnaudlimbourg · 2017-09-25T07:59:23Z

Anything we can do to help move this forward?

BlackVegetable · 2017-09-25T15:44:32Z

@arnaudlimbourg If you'd like to provide another set of eyes to the documentation changes, that'd be helpful. I'm going to be pinging some people within Keen to get other reviews as well in the very near future. Thanks for the reminder about this! (My wife had a baby very recently so this was put on the back-burner for a bit.)

masojus · 2017-09-25T19:59:08Z

keen/__init__.py

@@ -441,3 +441,125 @@ def get_all_collections():
    """
    _initialize_client_from_environment()
    return _client.get_all_collections()
+
+def create_access_key(name, is_active=True, permitted=[], options={}):


Was there a deliberate decision made to put this right in __init__.py instead of doing something liked the Saved/Cached Query Interface? Just curious.

The answer to all of these was, for the most part, to avoid deviating from the existing patterns I saw. The other answer is that I wanted to create the simplest path for users to run Access Key operations, which meant hiding the client (shoving it in __init__ like the basic queries) but also allowing them to work with a specific client in case they have multiple projects they are running in a single Python process.

To be honest though, I made that decision pretty lightly and could be persuaded to move it into its own interface. I don't like how monolithic that file has become.

I'm ambivalent, so whatever feels right to you.

masojus · 2017-09-25T20:01:34Z

keen/api.py

+
+        return response.json()
+
+    def _build_access_key_dict(self, access_key):


It seems this could be static, i.e. no need for the self param

I thought it was following a pre-existing pattern, and maybe it is but... we should probably run a cleanup pass through these functions that don't need self at some point in the near future -- including this one.

I expected the Codacity Code Analysis check to complain about it. Maybe that only runs on master? Not sure.

It did complain, but the same pattern exists elsewhere. I'll plan on having this merged in about 18 hours, but before I do so, I'll probably remove this erroneous self and leave other cleanup for later.

masojus · 2017-09-25T20:09:23Z

keen/api.py

+
+        self._error_handling(response)
+        return response.json()
+
    def _error_handling(self, res):


Not urgent, but it seems like _error_handling() should return the JSON since we end up repeating that step anyway.

I agree. This should be addressed at some point, if not now.

masojus · 2017-09-25T20:18:09Z

keen/api.py

@@ -186,7 +186,7 @@ def get_collection(self, event_collection):
    @requires_key(KeenKeys.READ)
    def get_all_collections(self):
        """
-        Extracts schema for all collections using the Keen IO API. A master key must be set first.
+        Extracts schema for all collections using the Keen IO API. A read key must be set first.


Thanks! I missed that.

masojus

Thanks @BlackVegetable!

tbarn

I commented a few small things in the readme. Docs look great - love the update! ✨

tbarn · 2017-09-26T03:11:52Z

README.rst

+
+.. code-block:: python
+
+    import keen


maybe from keen.client import KeenClient instead?

I'm not tied to that way, but for consistency?

Basic queries don't require the client to be used. Eventually I'd like to make it clear that you use the client object if you need to use different projects in the same python process, but otherwise you can just use the keen package to run your stuff. If you think it would be clearer within the docs to use the Client (and make a mention of the root package being usable for this as well) I can make that change.

tbarn · 2017-09-26T03:22:37Z

README.rst

+
+    # Master key must be set in an environment variable ahead of time.
+
+    # Create an access key.


In the comment, could you add a link to permissions section in the docs? It would be handy since there's a lot of different permissions.

Maybe this section? https://keen.io/docs/access/access-keys/#customizing-your-access-key

Thanks. Will do.

tbarn · 2017-09-26T03:24:16Z

README.rst

+
+    # Replace everything but the key ID with what is supplied here.
+    # If a field is not supplied here, it will be set to a blank value.
+    # In this case, no options are supplied, so any options will be removed.


"any" -> "all"

arnaudlimbourg · 2017-09-26T07:42:29Z

README.rst

+    keen.update_access_key_full(access_key_id="ABCDEFGHIJKLMNOPQRSTUVWXYZ", name="Strong_Bad", is_active=True, permitted=["queries"])
+
+
+Create Scoped Keys (Deprecated)


Maybe deprecated in bold to make it more obvious. Another option is to move in Legacy/Deprecated section

Good suggestion. For now I'll bold it. Later I'll likely do as you suggest with a dedicated section for such.

BlackVegetable · 2017-09-26T15:32:05Z

I'm going to look into the time cost of adding unit tests for these in a couple of days. So this won't be merged until at least then.

This does not include docs yet.

Changelog and cutting a new version.

This API will now support specific updates of access key names, options, and permissions. It also supports adding and removing some permissions from permission lists without requiring the user to replace the entire permissions list every time with exactly the end result he or she desires.

More tests. I've broken these tests out into their own file. An item of followup work this presents is to move the helper function and using a constant for a url prefix pattern to the remainder of the tests as well, but I feel that is beyond the scope of this already large issue. friendlier imports for non2.7 2.6 hates format without numbers

masojus · 2017-10-20T06:30:32Z

keen/tests/access_key_tests.py

+    def test_update_access_key_full(self, post):
+        # The update tests have a significant amount of logic that will not be tested via blackbox testing without
+        # un-mocking Keen's API. So this is the only test that will really cover any of them, and not even very
+        # well.


All of these tests seem to only make sure the URL is generated properly and that the client/api regurgitate the response that's given to the mock. This gets good CC numbers, but isn't really doing too much in terms of being more certain it'll work with the real back end. I realize that's how all the query tests and most of the existing tests in general are, and it's out of scope for this PR, but I wonder if we shouldn't have a push a to do some validation on the payload generated like we do in test_update_full_saved_query() and friends (see commit c4e3cae). If that seems like a good idea, I think it'd be worth filing an issue.

Yeah, I'm definitely not a fan of these tests as they are presently constituted. This repo definitely would benefit from some test enhancements.

masojus · 2017-10-20T06:59:35Z

keen/api.py

+
+        return response.json()
+
+    def _build_access_key_dict(access_key):


This is really a static method, so it could have the @staticmethod decorator, but maybe that's not necessary since this isn't meant to be called from outside the class, but in either case it's strange that this is invoked with self. in several places below when it doesn't take the self param.

Oh, yeah, that's a little weird. Lemme fix that.

We will no longer support python 2.6 or 3.3. Neither is being used heavily with this library according to pip data, and both are EOL (2.6 since 2013!)

BlackVegetable self-assigned this Sep 13, 2017

This was referenced Sep 13, 2017

Deprecate Python 3.2 Support #129

Closed

Add support for Access Keys #109

Closed

masojus mentioned this pull request Sep 20, 2017

Relax requirements on requests #133

Closed

masojus reviewed Sep 25, 2017

View reviewed changes

masojus approved these changes Sep 25, 2017

View reviewed changes

tbarn suggested changes Sep 26, 2017

View reviewed changes

arnaudlimbourg reviewed Sep 26, 2017

View reviewed changes

tbarn approved these changes Sep 26, 2017

View reviewed changes

tbarn mentioned this pull request Oct 16, 2017

Add support for Access Keys keenlabs/KeenClient-PHP#94

Open

BlackVegetable added 10 commits October 19, 2017 14:48

Added support for Access Keys.

72d2a5c

This does not include docs yet.

updated README with access keys

7104030

minor fixes

3a8ebda

Fixed Travis test dependencies

55953d4

Version 0.5.0

cba39a8

Changelog and cutting a new version.

Updated utils version string

abfa25c

better docs

966fff3

docs update from review

a6aed95

BlackVegetable force-pushed the issues-109 branch from 5882aac to 734b111 Compare October 19, 2017 20:50

BlackVegetable added 2 commits October 19, 2017 14:56

Actually update changelog

2e72d86

fixed 2.6 compat with order_by test

a672c89

masojus reviewed Oct 20, 2017

View reviewed changes

BlackVegetable added 2 commits October 20, 2017 09:44

+staticmethod decorator

1fc2b11

Deprecate 2.6 and 3.3

bd88aee

We will no longer support python 2.6 or 3.3. Neither is being used heavily with this library according to pip data, and both are EOL (2.6 since 2013!)

BlackVegetable merged commit 92512cb into master Oct 20, 2017

BlackVegetable deleted the issues-109 branch October 20, 2017 16:23

This was referenced Oct 26, 2017

Refactor _error_handling() #143

Open

Expand on Unit Tests #144

Open

Create Access Key keenlabs/keen-sdk-net#76

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues 109 #132

Issues 109 #132

BlackVegetable commented Sep 13, 2017

arnaudlimbourg commented Sep 25, 2017

BlackVegetable commented Sep 25, 2017

masojus Sep 25, 2017

BlackVegetable Sep 25, 2017

masojus Sep 25, 2017

masojus Sep 25, 2017

BlackVegetable Sep 25, 2017

masojus Sep 25, 2017

BlackVegetable Sep 25, 2017

masojus Sep 25, 2017

BlackVegetable Sep 25, 2017

masojus Sep 25, 2017

masojus left a comment

tbarn left a comment

tbarn Sep 26, 2017

BlackVegetable Sep 26, 2017

tbarn Sep 26, 2017

tbarn Sep 26, 2017

BlackVegetable Sep 26, 2017

tbarn Sep 26, 2017

BlackVegetable Sep 26, 2017

arnaudlimbourg Sep 26, 2017

BlackVegetable Sep 26, 2017

BlackVegetable commented Sep 26, 2017

masojus Oct 20, 2017 •

edited

Loading

BlackVegetable Oct 20, 2017

masojus Oct 20, 2017

BlackVegetable Oct 20, 2017


		return response.json()

		def _build_access_key_dict(self, access_key):


		# Master key must be set in an environment variable ahead of time.

		# Create an access key.

		keen.update_access_key_full(access_key_id="ABCDEFGHIJKLMNOPQRSTUVWXYZ", name="Strong_Bad", is_active=True, permitted=["queries"])


		Create Scoped Keys (Deprecated)


		return response.json()

		def _build_access_key_dict(access_key):

Issues 109 #132

Issues 109 #132

Conversation

BlackVegetable commented Sep 13, 2017

arnaudlimbourg commented Sep 25, 2017

BlackVegetable commented Sep 25, 2017

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

masojus left a comment

Choose a reason for hiding this comment

tbarn left a 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

BlackVegetable commented Sep 26, 2017

masojus Oct 20, 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

masojus Oct 20, 2017 •

edited

Loading