Add package and repository #43

colin-msphere · 2018-03-28T17:41:14Z

Add a package property to DCOS API Session. This returns a new Package object that maps directly to the "/package" context.

A repository property is available on Package that maps to "/package/repository" and provides methods to interact with the repository context.

colin-msphere · 2018-03-28T17:41:56Z

This is missing unit tests, which I will add soon.

margaret · 2018-03-30T17:22:17Z

dcos_test_utils/package.py

+        }
+        if version is not None:
+            params['packageVersion'] = version
+        if options and type(options) == dict:


Would it be better to raise a TypeError on these instead of silently skipping the param if it's not the expected type?

margaret · 2018-03-30T17:22:47Z

dcos_test_utils/package.py

+            'uri':  uri,
+            'name': name,
+        }
+        if index is not None and type(index) == int:


Raise a TypeError if it's not an int instead?

margaret · 2018-03-30T17:55:55Z

dcos_test_utils/package.py

+        if app_id:
+            params['appId'] = app_id
+        return self._make_request('install', params,
+                                  {'response_version': '2', })


Trailing comma is valid, although I am not using it well. I will remove it 👍

colin-msphere · 2018-04-02T17:39:09Z

Rebased.

falseperfection

So repetitive! Looks good. I had two questions but nothing big.

falseperfection · 2018-04-04T18:39:36Z

integration_test.py

+    assert found
+    pack_api.uninstall('hello-world', app_id=installed_id)
+
+    pack_api.describe('jenkins')


Is there no assert because these methods raise exceptions on failure? Looking through the implementation above it wasn't 100% obvious that _make_request couldn't just return empty when you expected output.

These methods will raise an HTTPError when a response is not "OK"-like. Because this is talking to an actual cosmos instance, I did not want to assert on content, as that may change or the server under test may have a custom universe installed (package repo).

A lack of raise is a decent check to ensure the request is valid.

falseperfection · 2018-04-04T18:40:53Z

tests/helpers.py

@@ -0,0 +1,69 @@
+from requests import HTTPError


+1 abstracting these utilities out

falseperfection · 2018-04-04T18:55:53Z

dcos_test_utils/package.py

+        :return: JSON response
+        :rtype: dict
+        """
+        params = {}


Is a param required? Or can someone do r.delete() with no params? If required, I'd raise an exception if neither is set like you do above with the int type check.

Not sure - thanks for the question. I need to run some tests.

falseperfection · 2018-04-09T17:52:08Z

integration_test.py

+    assert len(listings) > 0
+    old_name, old_uri = listings[0]['name'], listings[0]['uri']
+
+    repo.delete(old_name)


Verify the delete? Is this a possibility:

repo.delete(old_name) does nothing because of some error

repo.add(old_name, old_uri, 0) adds the repo a second time (or it silently skips, IDK the behavior)

assert says everything went fine because old_name is still there

I will add this.

colin-msphere · 2018-04-11T00:09:23Z

dcos-launch issue, but the tests passed.

margaret · 2018-04-13T17:15:07Z

Errors in the latest run of integration tests (on dcos-launch wait) are https://jira.mesosphere.com/browse/QUALITY-2025 which also appeared in #44 which only slowed down a retrying rate, so I think it's a previously introduced bug.

mellenburg

Great use of python and beautiful docs+testing.

Only change I would make is ensure that debug data is actually dumped in the unit tests

mellenburg · 2018-04-16T19:48:58Z

dcos_test_utils/package.py

@@ -93,3 +93,222 @@ def list_packages(self):
        """
        self._update_headers('list')
        return self._post('/list', {})
+
+
+class Repository(Cosmos):


You've went and gone full-python now 👍

mellenburg · 2018-04-16T19:51:05Z

tests/test_package.py

+    monkeypatch.setattr(requests, 'Session',
+                        lambda *args, **kwargs: mock_session)
+
+    yield mock_session


This should probably be wrapped in a try: finally block so that we always get the debug cache after an exception is raised

mellenburg · 2018-04-16T22:44:50Z

Errors are related to cluster cleanup which is done by dcos-launch, which is outside of this repo

Add a `package` property to DCOS API Session. This returns a new `Package` object that maps directly to the "/package" context. A `repository` property is available on Package that maps to "/package/repository" and provides methods to interact with the repository context.

Repository was using the wrong "post" method. Integration test for repository was using copy/pasted code that called a nonexistant method on `marathon`.

Move mocks to a helpers file so that both Jobs and Repository tests can use them.

Add more unit tests to verify URL and params. All package requests go through a single method instead of repeating the same code again and again. The default versions for endpoints is now 'v1' instead of 'v2'. The `install` path sets it to 'v2'.

colin-msphere added the enhancement label Mar 28, 2018

margaret reviewed Mar 30, 2018

View reviewed changes

colin-msphere force-pushed the add-repository branch from 3e59ebc to b491970 Compare April 2, 2018 17:38

falseperfection reviewed Apr 4, 2018

View reviewed changes

falseperfection reviewed Apr 9, 2018

View reviewed changes

colin-msphere force-pushed the add-repository branch from 50f0e91 to 2fff72d Compare April 16, 2018 19:39

mellenburg suggested changes Apr 16, 2018

View reviewed changes

mellenburg previously approved these changes Apr 16, 2018

View reviewed changes

colin-msphere added 15 commits April 19, 2018 10:18

Change request header version and content for package and repository

61cc847

Use correct "post" method and correct wait for deployment method

c723819

Repository was using the wrong "post" method. Integration test for repository was using copy/pasted code that called a nonexistant method on `marathon`.

Add tests for Repository

b97f3aa

Move mocks to a helpers file so that both Jobs and Repository tests can use them.

Fix the repository path

015da59

Improve integration tests for packages

8c961ff

Package list operation only accepts v1

873492f

Improvements to package and tests

173a7fa

Add more unit tests to verify URL and params. All package requests go through a single method instead of repeating the same code again and again. The default versions for endpoints is now 'v1' instead of 'v2'. The `install` path sets it to 'v2'.

alignment

ca4c8aa

Do not wait for package to uninstall

01dd786

Update response version for list and describe

f146e2b

Walk back version changes

0f4e3ff

Raise a TypeError when inputs are not supported types

d86341f

Add more tests to increase coverage

7601663

Add more package calls to integration test

e1e3dfb

colin-msphere added 9 commits April 19, 2018 10:18

Add type hint to package property

4eac218

Fix typo

e11a0ee

Use v2 for package describe response

9b5a5ee

Use Jenkins for package tests

bbeb2b6

Add to repo integration test

016771b

Repo integration test now uses existing repos

9e57929

name is required for repo delete

374892d

Check repo listings after each command

c29cf76

Wrap debug output in try/catch so it always gets printed

2868438

colin-msphere dismissed mellenburg’s stale review via 2868438 April 19, 2018 17:19

colin-msphere force-pushed the add-repository branch from 06eb9ee to 2868438 Compare April 19, 2018 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add package and repository #43

Add package and repository #43

colin-msphere commented Mar 28, 2018

colin-msphere commented Mar 28, 2018

margaret Mar 30, 2018

margaret Mar 30, 2018

margaret Mar 30, 2018

colin-msphere Mar 30, 2018

colin-msphere commented Apr 2, 2018

falseperfection left a comment

falseperfection Apr 4, 2018

colin-msphere Apr 4, 2018

falseperfection Apr 4, 2018

falseperfection Apr 4, 2018

colin-msphere Apr 4, 2018 •

edited

Loading

falseperfection Apr 9, 2018

colin-msphere Apr 9, 2018

colin-msphere commented Apr 11, 2018

margaret commented Apr 13, 2018 •

edited

Loading

mellenburg left a comment

mellenburg Apr 16, 2018

mellenburg Apr 16, 2018

mellenburg commented Apr 16, 2018

Add package and repository #43

Are you sure you want to change the base?

Add package and repository #43

Conversation

colin-msphere commented Mar 28, 2018

colin-msphere commented Mar 28, 2018

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

colin-msphere commented Apr 2, 2018

falseperfection 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

colin-msphere Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-msphere commented Apr 11, 2018

margaret commented Apr 13, 2018 • edited Loading

mellenburg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellenburg commented Apr 16, 2018

colin-msphere Apr 4, 2018 •

edited

Loading

margaret commented Apr 13, 2018 •

edited

Loading