Live tests, added missing features #54

jasmingacic · 2019-08-06T20:17:29Z

live tests for:

batches,
devices,
emails,
events,
ips
organizations,
ports,
vlans,
volumes and
VPN

Some of the features from the list were added to the SDK.

This change is

jasmingacic · 2019-08-06T20:19:23Z

Updated docs are going to be added here as well.

tox.ini

README.md

packet/BGPConfig.py

t0mk · 2019-08-08T13:06:15Z

packet/Manager.py

@@ -244,3 +259,376 @@ def validate_capacity(self, servers):
    def get_spot_market_prices(self, params={}):
        data = self.call_api("/market/spot/prices", params=params)
        return data["spot_market_prices"]
+


This is a lot of new functionality that works only in theory. An API SDK should contain Integration tests, at least for the most common use-cases. I mean tests that call the API.

https://github.com/TheAntColony/packet-python/blob/master/test/test_device.py#L40-L47 We tried to test where it was possible. BGP Configs were not tested, so we can either leave them or remove them.

jasmingacic · 2019-08-08T13:15:45Z

@t0mk The README still needs some work to be done. So we are going to revisit it again.

t0mk · 2019-08-09T08:22:59Z

@jasmingacic alright. I know that the original README has not been updated for a long time, and most of my comments address issues from the original format.
I am trying to see how it would be most useful for a user, and now that you're looking at packet-python, it's good time to improve it.

Feel free to give feedback on my code comments.

I will test some of the code today.

t0mk

I tried to run the tests, but they were creating resources in my project and organization. Project and Org for test should be created and destroyed after test. See comments.

t0mk · 2019-08-12T10:29:34Z

test/test_device.py

+    @classmethod
+    def setUpClass(self):
+        self.manager = packet.Manager(auth_token=os.environ['PACKET_AUTH_TOKEN'])
+        self.projectId = self.manager.list_projects()[0].id


Taking first project for tests will not do. You should create a temporary project and create test resources in it. In this case, the project should have BGP enabled.

This goes for all the resource tests which create project-owned resource.

The test project then should be destroyed at test tearDown, no matter if test succeeds or not.

You canb use setUp and tearDown() from python unittest:
https://docs.python.org/2/library/unittest.html#unittest.TestCase.setUp

The tests have been updated. Each test that requires a project resources creates a new project.

Great, it looks good. One more nitpick, could you please remove

datetime.utcnow().timestamp()

..from the test scripts? It doesn't work in Python2.7. The rest of the library works OK in both 2.7 and 3.x. Maybe replace it with something like:

datetime.utcnow().strftime("%Y%m%dT%H%M%S")

t0mk · 2019-08-12T10:34:06Z

test/test_organization.py

+    def setUpClass(self):
+        self.manager = packet.Manager(auth_token=os.environ['PACKET_AUTH_TOKEN'])
+        orgs = self.manager.list_organizations()
+        self.org_id = orgs[0].id


I think tests should not taint existing resoruce. Don't just use user's first organization. Rather create one in setUp and remove it in tearDown. Create test projects under the test organization.

I agree, the tests should use dedicated resources, including the organization. However, POST /organizations, as well as the integration test, are not implemented due to inability to provide a valid payment method. We can see the following message in the UI when creating a new organization:

We will temporarily place an authorization request on your credit card for $1. The charge will be removed within 5 business days.

sounds good

test/test_ips.py

t0mk · 2019-08-14T12:42:33Z

test/test_ips.py

+    def test_create_device_ip(self):
+        ip = None
+        params = {
+            "include": ["facility"]


include param should be a comma-separated string: https://www.packet.com/developers/api/#common-parameters

If you pass it like this, it will go to the GET as
GET /projects/cc99d24b-f23e-43e2-867e-275918dddcf3/ips?include=['facility']

It should be
GET /projects/cc99d24b-f23e-43e2-867e-275918dddcf3/ips?include=facility

The test has been updated.

t0mk · 2019-08-14T12:55:00Z

packet/Manager.py

+            "tags": tags
+        }
+
+        data = self.call_api("/projects/%s/ips" % project_id, params=request)


there should be type=POST. Otherwise this does GET and reads something useless from the API. Even worse, the method call succeeds, and the IP is not reserved.

t0mk · 2019-08-14T12:55:57Z

test/test_ips.py

+        cls.manager.reserve_ip_address(project_id=cls.project.id,
+                                       type="global_ipv4",
+                                       quantity=1,
+                                       facility="ewr1")


Please veify somehow that the global IP address is created.

Updated to use public_ipv4 instead of global_ipv4. public_ipv4 contains a broader IP address range.

t0mk · 2019-08-14T12:57:28Z

test/test_ips.py

+        for i in ips:
+            if i.facility.code == "ewr1" \
+                    and i.address_family == 4:
+                ip = i


Rather than lookup for an IP, please refer to the IP address allocated in the setUpClass method.

jasmingacic · 2019-08-16T09:24:08Z

@t0mk are we good to merge this?

mattcburns · 2019-08-16T16:13:38Z

packet/Device.py

        self.created_at = data["created_at"]
+        self.updated_at = data["updated_at"]
+        if "ipxe_script_url" in data:
+            self.ipxe_script_url = data["ipxe_script_url"]


This could be tightened up to be self.ipxe_script_url = data.get("ipxe_script_url", None) (this goes for the other if/else being done like this.)

Refactored. Nice catch @mattcburns

t0mk · 2019-08-18T20:32:28Z

Yeah, looks OK. Feel free to merge.

…ts, ips, organizations, ports, vlans, volumes and VPN

t0mk suggested changes Aug 8, 2019

View reviewed changes

t0mk reviewed Aug 12, 2019

View reviewed changes

nurfet-becirevic force-pushed the master branch from 6f91aa0 to 304ea48 Compare August 12, 2019 21:43

t0mk reviewed Aug 14, 2019

View reviewed changes

test/test_ips.py Show resolved Hide resolved

t0mk reviewed Aug 14, 2019

View reviewed changes

amenowanna requested a review from mattcburns August 16, 2019 16:01

mattcburns reviewed Aug 16, 2019

View reviewed changes

nurfet-becirevic force-pushed the master branch from 37f5916 to e8802ce Compare August 19, 2019 15:18

Added missing features and live tests: batches, devices, emails, even…

ef0db65

…ts, ips, organizations, ports, vlans, volumes and VPN

nurfet-becirevic force-pushed the master branch from e8802ce to ef0db65 Compare August 20, 2019 16:23

jasmingacic merged commit 7e55c71 into packethost:master Aug 20, 2019

mmlb mentioned this pull request Oct 4, 2019

CI configuration and PIP package automated updates #58

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Live tests, added missing features #54

Live tests, added missing features #54

jasmingacic commented Aug 6, 2019 •

edited by nathangoulding

Loading

jasmingacic commented Aug 6, 2019

t0mk Aug 8, 2019

jasmingacic Aug 9, 2019

jasmingacic commented Aug 8, 2019

t0mk commented Aug 9, 2019

t0mk left a comment

t0mk Aug 12, 2019

nurfet-becirevic Aug 12, 2019

t0mk Aug 14, 2019

nurfet-becirevic Aug 15, 2019

t0mk Aug 12, 2019

nurfet-becirevic Aug 12, 2019

t0mk Aug 14, 2019

t0mk Aug 14, 2019 •

edited

Loading

nurfet-becirevic Aug 15, 2019

t0mk Aug 14, 2019

nurfet-becirevic Aug 15, 2019

t0mk Aug 14, 2019

nurfet-becirevic Aug 15, 2019

t0mk Aug 14, 2019

nurfet-becirevic Aug 15, 2019

jasmingacic commented Aug 16, 2019

mattcburns Aug 16, 2019

nurfet-becirevic Aug 18, 2019

t0mk commented Aug 18, 2019

Live tests, added missing features #54

Live tests, added missing features #54

Conversation

jasmingacic commented Aug 6, 2019 • edited by nathangoulding Loading

jasmingacic commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmingacic commented Aug 8, 2019

t0mk commented Aug 9, 2019

t0mk 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

t0mk Aug 14, 2019 • 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

jasmingacic commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0mk commented Aug 18, 2019

jasmingacic commented Aug 6, 2019 •

edited by nathangoulding

Loading

t0mk Aug 14, 2019 •

edited

Loading