Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding documentation and object to 'create' function. #56

Merged
merged 15 commits into from
Jul 5, 2017
27 changes: 26 additions & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
Pygerduty
=========

Python Library for PagerDuty's v1 REST API.
Python Library for PagerDuty's REST API and Events API. This library was originally written to support v1 and
is currently being updated to be compatible with v2 of the API. See "Migrating from v1 to v2" for more details.

This library is currently evolving and backwards compatibility cannot always be guaranteed at this time.

Expand Down Expand Up @@ -42,16 +43,40 @@ Top level resources will be accessible via the PagerDuty object and nested
resources available on containers returned from their parent resource.


Migrating from v1 to v2
=======================

In order to allow for a smooth transition between versions 1 and 2 of the library,
version 1 library remains in the file called `__init__.py` inside of the pygerduty directory.
Also in that directory you will see four other files:

- `v2.py` — This file contains all updated logic compatible with v2 of the API.
- `events.py` — PygerDuty also provides an Events API which is separate from the REST API that has had the recent update. Since the logic is mostly disjoint, we have created a new module for logic related to the Events API.
- `common.py` — This file contains all common functions used by both `v2.py` and `events.py`.
- `version.py` — Contains version info.

See the examples below to see how this affects how you will instantiate a client in v2.


Examples
========

Instantiating a client:

Version 1:

::

import pygerduty
pager = pygerduty.PagerDuty("foobar", "SOMEAPIKEY123456")

Version 2:

::

import pygerduty.v2
pager = pygerduty.v2.PagerDuty("SOMEAPIKEY123456")

Listing a resource:

::
Expand Down
32 changes: 31 additions & 1 deletion pygerduty/v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,38 @@ def create(self, **kwargs):
if "requester_id" in kwargs:
extra_headers["From"] = kwargs.pop("requester_id")

data[self.sname] = kwargs
new_kwargs = {}

for kwarg_key, kwarg_value in kwargs.iteritems():
if kwarg_key.endswith('_id'):
new_kwargs = Collection.id_to_obj(kwarg_key, kwarg_value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you're overwriting new_kwargs here? Can you move this entire loop into a function and add a test for it to make sure we're not losing kwargs? The test should have kwargs that are and aren't transformed and test both id and ids types.

elif kwarg_key.endswith('_ids'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe if it ends in _ids it will be a list of ids ["ID1", "ID2"] that needs to be translated to a list of objects

[{"type": "sometype", "id": "ID1"}, ...]

This seems to be doing the same thing as _id

new_kwargs = Collection.ids_to_objs(kwarg_key, kwarg_value)
else:
new_kwargs[kwarg_key] = kwarg_value
data[self.sname] = new_kwargs

response = self.pagerduty.request("POST", path, data=_json_dumper(data), extra_headers=extra_headers)
return self.container(self, **response.get(self.sname, {}))

@staticmethod
def id_to_obj(key, value):
new_key = key[:-3]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmjosack I struggled with figuring out where to put the string slice. Originally I left it inside of the create function but found that hard to test. Putting it in both id_to_obj and ids_to_objs it caused the key to be sliced twice since ids_to_objs depends on id_to_obj. It created something like this:

[{'id': 'PF9KMXH', 'type': 'servi'}, {'id': 'PIJ90N7', 'type': 'servi'}]

My solution was to only do it in id_to_obj, but then I realized we check for '_ids' two times (here and in create function) and this did not see very dry. Is there a better way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One option is to only pass values to these functions and worry about renaming the key in the outer function that I suggested.

if key.endswith('_ids'):
new_key = key[:-4] + "s"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any resources that end in y that might be ies instead of s? e.g. policy_ids -> policies. I believe we have a pluralize function that could be reused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see anywhere where policy_ids are used in the API reference. The v1 reference actually shows:

  "escalation_rules": [
    {
      "id": "PYO9B13",
      "escalation_delay_in_minutes": 22,
      ...
       }
    }

compared to in v2:

{
  "escalation_policies": [
    {
      "id": "PANZZEQ",
      "type": "escalation_policy",
      ...
    }
}

return {
"id": value,
"type": new_key
}

@staticmethod
def ids_to_objs(key, value):
new_kwargs = []
for v in value:
new_kwarg = Collection.id_to_obj(key, v)
new_kwargs.append(new_kwarg)
return new_kwargs

def update(self, entity_id, **kwargs):
path = "{0}/{1}".format(self.name, entity_id)
if self.base_container:
Expand Down Expand Up @@ -268,6 +295,9 @@ def update(self, entity_id, **kwargs):


class ScheduleUsers(Collection):
"""This class exists because Users returned from a Schedule query are not
paginated, whereas responses for Users class are. This causes a pagination
bug if removed."""
name = 'users'
paginated = False

Expand Down
43 changes: 43 additions & 0 deletions tests/collection_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
from __future__ import absolute_import

import pygerduty.v2

###################
# Version 2 Tests #
###################

def test_id_to_obj():

kwarg = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these tabs for indents?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been having issues with my tabs, sometimes it does tabs = four spaces and other times it does not. I will check on it.

"escalation_policy_id": "PIJ90N7",
}

new_kwarg = pygerduty.v2.Collection.id_to_obj("escalation_policy_id", kwarg["escalation_policy_id"])

assert new_kwarg == {
"id": "PIJ90N7",
"type": "escalation_policy"
}


def test_ids_to_objs():

kwarg = {
"service_ids": [
"PF9KMXH",
"PIJ90N7"
]
}

new_kwargs = pygerduty.v2.Collection.ids_to_objs("service_ids", kwarg["service_ids"])

assert new_kwargs == [
{
"id": "PF9KMXH",
"type": "services"
},
{
"id": "PIJ90N7",
"type": "services"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type should be service here I believe. It's the key to this list that needs to be plural that's passed to the api.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmjosack Actually I just looked back and this is the reference thing we talked about before:

"services": [
      {
        "id": "PIJ90N7",
        "type": "service_reference"
      }
    ],

You said that you think it won't mind right? I will go ahead and change it to service if you think that will work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, pretty sure it happily takes either service or service_reference but I believe it has to be singular.

}
]