Add support for GPG keys #886

jacquerie · 2018-08-13T16:48:20Z

Implement the CRUD API for users' GPG keys described
in https://developer.github.com/v3/users/gpg_keys/.

Closes #704

jacquerie · 2018-08-13T16:56:56Z

src/github3/users.py

+        if 'primary' in email:
+            self.primary = email['primary']
+        if 'visibility' in email:
+            self.visibility = email['visibility']


This is because the emails returned by the GPG key API only have the email and the verified attributes (see: https://developer.github.com/v3/users/gpg_keys/#response-2). An alternative implementation is to split the Email model into ShortEmail and Email; I chose the above solution for simplicity, but I'm open to suggestions!

I'd rather consistency over simplicity. That means creating ShortEmail and Email classes.

Sure thing! Will fix.

sigmavirus24 · 2018-08-15T13:36:16Z

src/github3/users.py

+        self.emails = [Email(email, self) for email in key['emails']]
+        try:
+            self.expires_at = self._strptime(key['expires_at'])
+        except ValueError:


Is this not valid in practice?

What can happen here is that key['expires_at'] might be null, for example https://developer.github.com/v3/users/gpg_keys/#response-2. I'm not sure what github3.py usually does when a value might be null, I originally had self.expires_at = None...

I thought that _strptime() handled that case automagically. If not, it should.

Whoops, you are right! See:

github3.py/src/github3/models.py

Lines 87 to 100 in aba7b23

@classmethod

def _strptime(cls, time_str):

"""Convert an ISO 8601 formatted string to a datetime object.

We assume that the ISO 8601 formatted string is in UTC and we create

the datetime object so that it is timezone-aware.

:param str time_str: ISO 8601 formatted string

:returns: timezone-aware datetime object

:rtype: datetime or None

"""

if time_str:

return dateutil.parser.parse(time_str)

return None

I was probably thinking of datetime's strptime. Will fix this too!

sigmavirus24 · 2018-08-15T13:37:02Z

src/github3/users.py

+        if 'primary' in email:
+            self.primary = email['primary']
+        if 'visibility' in email:
+            self.visibility = email['visibility']


I'd rather consistency over simplicity. That means creating ShortEmail and Email classes.

jacquerie · 2018-08-18T10:47:53Z

@sigmavirus24 I did what you asked in the review and I filled in the missing documentation TODOs. I think this is ready for another round of review!

sigmavirus24

Just a couple small changes to the tests. I really need to write better/updated documentation for the testing in this library

sigmavirus24 · 2018-08-18T11:31:12Z

tests/integration/test_github.py

+        self.token_login()
+        cassette_name = self.cassette_name('create_gpg_key')
+        with self.recorder.use_cassette(cassette_name):
+            gpg_key = self.gh.create_gpg_key(GPG_KEY)


In the past we've been a bit sloppy. Going forward I've started writing tests like so:

def test_create...(self): self.token_login() cassette_name = self.cassette_name('...') with self.recorder.use_cassette(...): gpg_key = self.gh.create_gpg_key(GPG_KEY) assert isinstance(gpg_key, github3.users.GPGKey) assert gpg_key.delete()

This way there's no out-of-band, manual clean-up required. Then for the

def test_gpg_key(self): ... with ...: created_gpg_key = self.gh.create_gpg_key(...) assert isinstance(created_gpg_key, ...) retrieved_gpg_key = self.gh.gpg_key(created_gpg_key.id) assert isinstance(retrieved_gpg_key, ...) assert created_gpg_key == retrieved_gpg_key assert created_gpg_key.delete(0

And the same goes for the last method too.

Uh, I like the idea of having each test responsible for its cleanup. Another benefit is that anybody can re-record the cassette if need arises. Done!

sigmavirus24 · 2018-08-18T11:31:47Z

tests/integration/test_users.py

@@ -73,6 +88,17 @@ def test_following(self):
        for person in following:
            assert isinstance(person, github3.users.ShortUser)

+    def test_gpg_keys(self):


Err... I can't decipher this comment! Do you want me to delete this test? It makes sense to me: it shows that an unauthenticated user can retrieve another user and ask for their GPG keys.

No, sorry. I was clapping as in "this is great"

sigmavirus24 · 2018-08-18T11:32:03Z

tests/integration/test_users.py

+        self.token_login()
+        cassette_name = self.cassette_name('delete')
+        with self.recorder.use_cassette(cassette_name):
+            gpg_key = self.gh.gpg_key(407238)


And this test can look a lot like the create_gpg_key test. It's good to have it anyway.

Implement the CRUD API for users' GPG keys described in https://developer.github.com/v3/users/gpg_keys/. Closes sigmavirus24#704

jacquerie commented Aug 13, 2018

View reviewed changes

jacquerie changed the title ~~Add support for GPG keys~~ [WIP] Add support for GPG keys Aug 13, 2018

sigmavirus24 approved these changes Aug 15, 2018

View reviewed changes

jacquerie force-pushed the add-gpg-keys branch 6 times, most recently from f46b683 to 9f08f95 Compare August 18, 2018 10:41

jacquerie changed the title ~~[WIP] Add support for GPG keys~~ Add support for GPG keys Aug 18, 2018

sigmavirus24 requested changes Aug 18, 2018

View reviewed changes

jacquerie force-pushed the add-gpg-keys branch from 9f08f95 to 5bd94db Compare August 18, 2018 18:05

Add support for GPG keys

6506039

Implement the CRUD API for users' GPG keys described in https://developer.github.com/v3/users/gpg_keys/. Closes sigmavirus24#704

jacquerie force-pushed the add-gpg-keys branch from 5bd94db to 6506039 Compare August 18, 2018 18:19

sigmavirus24 approved these changes Aug 19, 2018

View reviewed changes

sigmavirus24 merged commit 5eff8c6 into sigmavirus24:master Aug 19, 2018

jacquerie deleted the add-gpg-keys branch August 19, 2018 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for GPG keys #886

Add support for GPG keys #886

jacquerie commented Aug 13, 2018

jacquerie Aug 13, 2018 •

edited

Loading

sigmavirus24 Aug 15, 2018

jacquerie Aug 15, 2018

sigmavirus24 Aug 15, 2018

jacquerie Aug 15, 2018

sigmavirus24 Aug 15, 2018

jacquerie Aug 15, 2018

sigmavirus24 Aug 15, 2018

jacquerie commented Aug 18, 2018

sigmavirus24 left a comment

sigmavirus24 Aug 18, 2018

jacquerie Aug 18, 2018

sigmavirus24 Aug 18, 2018

jacquerie Aug 18, 2018 •

edited

Loading

sigmavirus24 Aug 19, 2018

sigmavirus24 Aug 18, 2018

	@classmethod
	def _strptime(cls, time_str):
	"""Convert an ISO 8601 formatted string to a datetime object.

	We assume that the ISO 8601 formatted string is in UTC and we create
	the datetime object so that it is timezone-aware.

	:param str time_str: ISO 8601 formatted string
	:returns: timezone-aware datetime object
	:rtype: datetime or None
	"""
	if time_str:
	return dateutil.parser.parse(time_str)
	return None

Add support for GPG keys #886

Add support for GPG keys #886

Conversation

jacquerie commented Aug 13, 2018

jacquerie Aug 13, 2018 • 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

jacquerie commented Aug 18, 2018

sigmavirus24 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

jacquerie Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacquerie Aug 13, 2018 •

edited

Loading

jacquerie Aug 18, 2018 •

edited

Loading