search

mozilla-services / pagerstatus

A service to automatically update Statuspage.io based on Pagerduty incidents
Apache License 2.0
7 stars 5 forks source link

Code review #7

Closed autrilla closed 6 years ago

autrilla commented 6 years ago

Not sure what other medium I could use for this. All my comments are nits, so consider this an r+ :)

--

Use the params kwarg instead of encoding it in the URL

https://github.com/mozilla-services/pagerstatus/blob/b22993cfe1e0a9cf3e31c68474a9340ca593cbb3/chalicelib/pagerduty.py#L17

--

I wouldn't name this module chalicelib unless you have to, as it's not really related to chalice. I'd just name it pagerstatus.

https://github.com/mozilla-services/pagerstatus/tree/b22993cfe1e0a9cf3e31c68474a9340ca593cbb3/chalicelib

--

Use f-strings here too for consistency

https://github.com/mozilla-services/pagerstatus/blob/master/chalicelib/statuspage.py#L8

--

Could do

r = requests.request(method, url, headers=headers, data=data)

or even

r = getattr(requests, method)(url, headers=headers, data=data)

https://github.com/mozilla-services/pagerstatus/blob/b22993cfe1e0a9cf3e31c68474a9340ca593cbb3/chalicelib/statuspage.py#L10

--

Having an Incident class for StatusPage might be good, e.g. for logging nicely and just encapsulating the states and behaviors attached to incidents. There's log messages like this one that provide no context:

https://github.com/mozilla-services/pagerstatus/blob/b22993cfe1e0a9cf3e31c68474a9340ca593cbb3/chalicelib/statuspage.py#L50

sciurus commented 6 years ago

Thanks for the review!

Chalice requires the name chalicelib for packaging to work, so I can't change that.

I agree the log messages currently don't make sense in isolation, they depend on context provided by logs in other parts of the code. I suspect you're right that there is a strong enough concept of an incident in the statuspage module to make it worth a class. That could be a worthwhile refactoring.

I've implemented all the other suggestions.