All users were logged out of Bugzilla on October 13th, 2018

Make using HawkAuth with the client more intuitive

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
See bug 1209555 comment 5.

Using HawkAuth with the client requires a little more boilerplate (and in not as clean a form as I'd like), so we need to decide what to do before documenting it and then asking people to migrate.

The options:

1) Use HawkAuth as is.

2) Try to improve upstream (see https://github.com/mozilla-services/requests-hawk/issues/8)

3) Provide a wrapper around HawkAuth ourselves (see https://github.com/mozilla/treeherder/compare/hawkauth-wrapper)
(Assignee)

Comment 1

3 years ago
Created attachment 8673353 [details] [review]
Allow passing Hawk client_id/secret directly to the client

I think I prefer this approach over making users of the client deal with the auth directly. I know things like requests make people pass in an `auth` object, but I think that is something that should be abstracted away from the user for treeherder-client.

This approach means we maintain maximum control over the use of HawkAuth, and can keep a stable API for users of the client, if we choose to switch to another library or HawkAuth adds support for the simpler constructor mentioned in https://github.com/mozilla-services/requests-hawk/issues/8 (and we don't then have to block on that GitHub issue).
Attachment #8673353 - Flags: review?(mdoglio)
(Assignee)

Updated

3 years ago
Attachment #8673353 - Flags: feedback?(wlachance)
Comment on attachment 8673353 [details] [review]
Allow passing Hawk client_id/secret directly to the client

I like this approach.

Maybe some tests would be nice?

At some point in the future we should replace the deprecation warnings with exceptions.
Attachment #8673353 - Flags: feedback?(wlachance) → feedback+
Attachment #8673353 - Flags: review?(mdoglio) → review+
(Assignee)

Comment 3

3 years ago
(In reply to William Lachance (:wlach) from comment #2)
> I like this approach.
> 
> Maybe some tests would be nice?

Agreed, whilst the etl tests do exercise this code, I've also added a test to test_treeherder_client.py and updated the PR.

> At some point in the future we should replace the deprecation warnings with exceptions.

Shortly they will be removed entirely - I was just wanting to release at least one version of the client that supports both forms of authentication, for easier transition. We haven't actually released a client to PyPI that supports Hawk auth yet - I was waiting until we had this bug fixed.
(Assignee)

Comment 4

3 years ago
(In reply to Ed Morley [:emorley] from comment #3)
> We haven't actually released a client to PyPI that
> supports Hawk auth yet - I was waiting until we had this bug fixed.

Well, in the form proposed here at least :-)

Comment 5

3 years ago
Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/b212683e2fd9d19da82fa36aa4028c20ba3c8e92
Bug 1213857 - Fix treeherder-client's setup.py requirements

Since requests-hawk had leading whitespace, and missed the review
comment. We also need at least requests 2.4.3, since we use the `json`
argument to `requests.post()`.

https://github.com/mozilla/treeherder/commit/892a6d975182cc082eb317e3301dbf4ff355e3ea
Bug 1213857 - Allow passing Hawk client_id/secret directly to the client

So people don't have to deal with HawkAuth themselves. The existing
`auth` is deprecated and will be removed in the future.

https://github.com/mozilla/treeherder/commit/9f84698e470d3fe27e55201d7e68188344584aca
Bug 1213857 - Update tests still using TreeherderAuth to use client_id

TreeherderAuth is deprecated, so this switches these tests to use
the new hawk client_id and secret parameters instead.
(Assignee)

Updated

3 years ago
Blocks: 1212936
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1214618
You need to log in before you can comment on or make changes to this bug.