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 api for repositories and tasks and few repository tests #20

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,7 @@ __pycache__/
# Coverage information
/.coverage
/htmlcov/

# Vi(m)'s swap files
*.swp
*.swo
Copy link
Contributor

Choose a reason for hiding this comment

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

These swapfiles are produced because an individual developer chose to use Vim, not because the project necessitated the use of Vim. Consequently, they are best ignored in the individual developer's Git configuration, not in the project-wide Git configuration.

You can do this like so:

cat >>~/.gitconfig <<EOF
[core]
    excludesfile = /home/peterlacko/.gitignore_global
EOF
echo '*.swp' > ~/.gitignore_global

4 changes: 4 additions & 0 deletions pulp_smash/resources/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# coding=utf-8
"""Modules for easy manipulation with pulp resources like users, repositories
etc."""
Copy link
Contributor

Choose a reason for hiding this comment

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

We can word-smith this and make it fit on one line. How about:

"""Modules for manipulating pulp resources like users, repositories, etc."""

Copy link
Contributor

Choose a reason for hiding this comment

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

The pulp_smash.resources.platform and pulp_smash.resources.platform.api_v2 modules have identical docstrings. As a result, the purpose of each module is not clear. Other developers will be asking themselves questions like "where does my new helper code go in this hierarchy of modules," and I'd like it if they can answer such questions by reading the docstrings for these modules.

Copy link
Author

Choose a reason for hiding this comment

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

Since it's very likely that big portion of code and it's organization will change I didn't pay much attention to docstrings in here, partly also because I wasn't sure about purpose of each module. I agree about docstrings, one-liner looks better.

from __future__ import unicode_literals
4 changes: 4 additions & 0 deletions pulp_smash/resources/platform/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# coding=utf-8
"""Modules for easy manipulation with pulp resources like users, repositories
etc."""
from __future__ import unicode_literals
4 changes: 4 additions & 0 deletions pulp_smash/resources/platform/api_v2/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# coding=utf-8
"""Modules for easy manipulation with pulp resources like users, repositories
etc."""
from __future__ import unicode_literals
183 changes: 183 additions & 0 deletions pulp_smash/resources/platform/api_v2/api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
# coding=utf-8
"""Api package contains classes for easier resource managament in pulp-smash.
"""

from __future__ import unicode_literals

import requests
import time
from requests.exceptions import HTTPError
from pulp_smash.config import get_config

# List of all paths here
CREATE_REPOSITORY_PATH = "/pulp/api/v2/repositories/"
Copy link
Contributor

Choose a reason for hiding this comment

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

This path is also used for searching. How about making a single constant like so:

REPOSITORY_PATH = '/pulp/api/v2/repositories/'

This path can then be re-used throughout. If you'd really like to have separate constants for repositories in general and specific repositories, how about this?

REPOSITORY_PATH = '/pulp/api/v2/repositories/'
REPOSITORY_ID_PATH = REPOSITORY_PATH + '{}/'

REPOSITORY_PATH = "/pulp/api/v2/repositories/{}/" # .format(<repo_id>)
REPOSITORY_PATH = "/pulp/api/v2/repositories/{}/" # .format(<repo_id>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate code.

POLL_TASK_PATH = "/pulp/api/v2/tasks/{}/" # .format(<task_id>)

# Repository related variables
REPORT_KEYS = {
'result',
'error',
'spawned_tasks',
}
ERROR_KEYS = {
'_href',
'error',
'error_message',
'exception',
'http_status',
'traceback',
}

# Task related variables
TASK_ERROR_STATES = {
'error',
'timed out',
}
TASK_FINISHED_STATES = {
'finished',
}


class Repository(object):
"""Provides interface for easy manipulation with pulp repositories.
`Create repo` accepts following kwarg parameters:
.. _Create repo:
http://pulp.readthedocs.org/en/latest/dev-guide/integration/rest-api/repo/cud.html

Each time request to server is made, ie. by calling :meth:`create_repo`
method, response is saved to last_response variable.

:param id: System wide unique repository identifier.
:param display_name: User-friendly name for the repository
:param description: User-friendly text describing the repository’s contents
:param notes: Key-value pairs to programmatically tag the repository
:param importer_type_id: Type id of importer being associated with the
repository
:param importer_config: Configuration the repository will use to drive
the behavior of the importer
:distributors: Array of objects containing values of distributor_type_id,
repo_plugin_config, auto_publish, and distributor_id
"""

def __init__(self, **kwargs):
self.data_keys = kwargs
self.last_response = None
self.cfg = get_config()

def create_repo(self, **kwargs):
"""Create repository on pulp server.
After calling this method, <repo>.last_response.raise_for_status()
should be called in order to make sure that repo was correctly created.
:param kwargs: Additional arguments which will be passed to request,
same as in :class:`Repository` constructor.
"""
self.data_keys.update(kwargs)
self.last_response = requests.post(
self.cfg.base_url + CREATE_REPOSITORY_PATH,
json=self.data_keys,
**self.cfg.get_requests_kwargs()
)

def delete_repo(self):
"""Delete repository from pulp server.
After calling this method, <repo>.last_response.raise_for_status()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is syntactically valid, but its semantics are probably not what you intend. reStructuredText denotes a new paragraph with a blank line, and there is no blank line here, so this is interpreted as one giant paragraph. See A reStructuredText Primer.

Taks.wait_for_tasks(<repo>.last_response) should be called in order
to make sure repo was correctly deleted.
"""
self.last_response = requests.delete(
self.cfg.base_url +
REPOSITORY_PATH.format(self.data_keys['id']),
**self.cfg.get_requests_kwargs()
)

def get_repo(self):
"""Get information about repository on server.
After calling this method, <repo>.last_response.raise_for_status()
should be called in order to make sure that call was succesfull.
"""
self.last_response = requests.get(
self.cfg.base_url + REPOSITORY_PATH.format(self.data_keys['id']),
**self.cfg.get_requests_kwargs()
)

def update_repo(self,
delta,
importer_config=None,
distributor_configs=None):
"""Update repository with keys from kwargs.
After calling this method, <repo>.last_response.raise_for_status()
and Task.wait_for_tasks(<repo>.last_response)
should be called in order to make sure repo was correctly updated.
:param delta: Object containing keys with values that should
be updated on the repository.
:param importer_config: Object containing keys with values that should
be updated on the repository’s importer config.
:param distributor_configs: object containing keys that
are distributor ids
"""
my_delta = {'delta': delta}
if importer_config is not None:
my_delta.update({'importer_config': importer_config})
if distributor_configs is not None:
my_delta.update({'distributor_configs': distributor_configs})
self.last_response = requests.put(
self.cfg.base_url + REPOSITORY_PATH.format(self.data_keys['id']),
json=my_delta,
**self.cfg.get_requests_kwargs()
)


class Task(object):
"""Handles tasks related operations. So far only waiting for given tasks
to immediate finish is implemented.
"""
cfg = get_config()
Copy link
Contributor

Choose a reason for hiding this comment

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

In my experience, class attributes are deceptive. They seem simple when you start using them, but become a real pain down the road when more use cases for your class arise. Here's two specific issues:

  • Let's say a user installs Pulp Smash for the very first time and executes make docs-html. As part of the documentation generation process, this module is imported and get_config is called. Since the user has just installed Pulp Smash, a config will not be found, and a ConfigFileNotFoundError is raised. 😢
  • Let's say we have two Pulp servers, and we want to monitor a task on each of them. It would be nice if we could instantiate two Task objects, give each a unique cfg object and call wait_for_task() on each. Using a class attribute makes this impossible.

Copy link
Author

Choose a reason for hiding this comment

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

I see, will change it.


def __init__(self):
pass

@classmethod
def _wait_for_task(cls, task, timeout, frequency):
"""Wait for single task to finish its execution on server.
:param task: Dictionary containtin task_id and path to task
on pulp server.
:param timeout: Timeout in seconds for each task to complete.
:param frequency: Task polling frequency in seconds.
"""
task_timeout = time.time() + timeout
while time.time() <= task_timeout:
time.sleep(frequency)
response = requests.get(
Task.cfg.base_url +
POLL_TASK_PATH.format(task["task_id"]),
**Task.cfg.get_requests_kwargs()
)
try:
response.raise_for_status()
except HTTPError:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is below:

            if response.json()["state"] in TASK_FINISHED_STATES:
                break

As a result, an HTTP error code looks the same as task success to a caller. How about just letting this exception propagate up instead of catching it?

Copy link
Author

Choose a reason for hiding this comment

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

I already rewrote that, I also didn't find it as a good solution (relates also to previous comment about raising Exception)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand the wording here. What happened?

Copy link
Author

Choose a reason for hiding this comment

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

I meant that I changed it like this:

while time.time() <= task_timeout:
    time.sleep(frequency)
    response = requests.get(
         self.cfg.base_url + POLL_TASK_PATH.format(task["task_id"]),
         **self.cfg.get_requests_kwargs()
     )
     response.raise_for_status()
     # task finished (with success or failure)
     if (response.json()["state"]
             in TASK_ERROR_STATES | TASK_SUCCESS_STATES):
         return response

so we can we can check response manually for error later (maybe error was expected?) and also Exception raising was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, now I understand. Thanks for explaining. 😄

# task finished with error
if response.json()["state"] in TASK_ERROR_STATES:
raise Exception("Error occured while polling task: ",
response.text)
# task finished properly
if response.json()["state"] in TASK_FINISHED_STATES:
break
# task probably timed out
else:
raise Exception("Timeout occured while waiting for task: ",
Copy link
Contributor

Choose a reason for hiding this comment

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

Never raise Exception. It's a Bad Thing™ to do. If a client wants to catch this exception, they will write something like this:

try:
    Task.wait_for_task()
except Exception:
    pass  # properly handle exception

This is problematic because Exception is the parent class of most exceptions. This code might catch completely unrelated exceptions.

response.text)

@classmethod
def wait_for_tasks(cls, report, timeout=120, frequency=0.5):
"""Wait for all populated tasks to finish.
:param report: Call response -- report -- with list of populated tasks.
:param timeout: Timeout in seconds for each task to complete.
:param frequency: Task polling frequency in seconds.
"""
if not all(key in report.json().keys() for key in REPORT_KEYS):
raise Exception("Missing key in Call report: ", report.text)
for task in report.json()["spawned_tasks"]:
cls._wait_for_task(task, timeout, frequency)
174 changes: 174 additions & 0 deletions pulp_smash/tests/platform/api_v2/simple_repo_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
# coding=utf-8
"""Test for basic repo creating functionality.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

If a docstring fits on one line, do this:

"""Ask a foo for a bar."""

If a docstring has a summary line and a body, do this:

"""Ask a foo for a bar.

Ensure that the bar has a biz and a baz.

"""

Copy link
Author

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've reread pep8 a few days ago, and figured out that the recommended way for multiline docstring is like example below ( see https://www.python.org/dev/peps/pep-0008/#documentation-strings for more information):

"""Return a foobang

Optional plotz says to frobnicate the bizbaz first.
"""

In other words, the closing """ should stay on its own line and don't need a blank line before it.

I'm commenting because I am using the PEP8 recommend format. But as the PEP8 document states, it is a code style guide not a rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

@elyezer Thanks for the heads up. See #24

from __future__ import unicode_literals

from pulp_smash.resources.platform.api_v2.api import Repository, Task
from pulp_smash.resources.platform.api_v2.api import ERROR_KEYS
from unittest2 import TestCase


class RepoCreateSuccessTestCase(TestCase):
"""Tests for successfull repo creating functionality.
"""

@classmethod
def setUpClass(cls):
"""Create repo on pulp server
"""
cls.repo = Repository(id=cls.__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if I cancel this test before tearDownClass runs, or if the repository deletion fails? In that case, I am unable to run this test again, because a repository with an identical ID already exists. Using a random string for the repository ID lets me avoid this issue, and allows me to parallelize test cases.

Copy link
Author

Choose a reason for hiding this comment

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

I was also thinking about this case and thought about trying to remove repo in setUpClass(), but similar problems could appear, so random ID makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

cls.repo.create_repo()

def test_status_code(self):
"""Test if Create repo returned 201."""
self.assertEqual(self.repo.last_response.status_code, 201)

def test_correct_id(self):
"""Test if response contain correct repo id.
"""
self.assertEqual(
self.repo.last_response.json()['id'],
self.__class__.__name__,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid using names like this. You can access an object's class with type(self) instead of self.__class__. And the __name__ reference can be dropped in favor of randomly generating an ID, which has benefits as described here.

set(self.repo.last_response.json()))
Copy link
Contributor

Choose a reason for hiding this comment

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

pep8 allows for quite a bit of flexibility in formatting, and there are no great code formatters for Python as in other languages, like Go and C. As a result, many different styles have emerged.

Please use the following layout for all method and function calls unless the situation strongly calls for something different. I know it's nit-picky of me to ask for this. But this layout is formulaic enough to work nearly everywhere, and using a consistent style makes it possible for people to easily scan through code.

# Yes.
self.assertEqual(
    self.repo.last_response.json()['id'],
    self.__class__.__name__,
    set(self.repo.last_response.json())
)
self.assertEqual(self.repo.last_response.json()['id'], self.__class__.__name__)

# No: closing parenthesis is not at same indentation as start of statement.
self.assertEqual(
    self.repo.last_response.json()['id'],
    self.__class__.__name__,
    set(self.repo.last_response.json()))
# No: hanging arguments are used.
self.assertEqual(self.repo.last_response.json()['id'],
                 self.__class__.__name__,
                 set(self.repo.last_response.json()))
# No: multiple arguments on same line
self.assertEqual(
    self.repo.last_response.json()['id'], self.__class__.__name__,
    set(self.repo.last_response.json()
)

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will try to follow it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.


@classmethod
def tearDownClass(cls):
"""Delete previously created repository.
"""
cls.repo.delete_repo()
cls.repo.last_response.raise_for_status()
Task.wait_for_tasks(cls.repo.last_response)


class RepoCreateMissingIdTestCase(TestCase):
"""Tests for repo create functionality with missing required data keys(id).
"""

@classmethod
def setUpClass(cls):
"""Create Repository with required data key missing.
"""
cls.repo = Repository(display_name=cls.__name__)
cls.repo.create_repo()

def test_status_code(self):
"""Check that request returned 400: invalid parameters.
"""
self.assertEqual(self.repo.last_response.status_code, 400,
self.repo.last_response.json())

def test_body(self):
"""Test if request returned correct body.
"""
self.assertLessEqual(
ERROR_KEYS,
set(self.repo.last_response.json().keys()),
self.repo.last_response.json())


class RepoExistsTestCase(TestCase):
"""Test if created repo exists on server.
"""

@classmethod
def setUpClass(cls):
"""Create repository on server with id, description and display_name,
test correct status code and get it.
"""
cls.repo = Repository(
id=cls.__name__,
display_name=cls.__name__,
description=cls.__name__
)
cls.repo.create_repo()
cls.repo.last_response.raise_for_status()
cls.repo.get_repo()

def test_status_code(self):
"""Test if server returned 200.
"""
self.assertEqual(self.repo.last_response.status_code, 200,
self.repo.last_response.json())

def test_body(self):
"""Test if repo has all set attributes: id, description and display_name.
"""
self.assertTrue(
all(self.repo.last_response.json()[key] == self.__class__.__name__
for key in ['id', 'display_name', 'description']))
Copy link
Contributor

Choose a reason for hiding this comment

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

This works. But you'll get more useful assertion error messages if you first check that the correct keys are present before checking the value of those keys.

Imagine for a moment that this assertion fails with a KeyError, and exception states that the no "display_name" attribute exists. In this case, you know that the server didn't return a "display_name" key, but you're left wondering what other attributes the server returned. Did the server return a "description" key? An assertion like this tells you for sure:

self.assertLessEqual(
    {'id', 'display_name', 'description'},
    keys(self.repo.last_response.json())
)

The assertion will raise an exception stating something like "elements in the first set but not the second: 'display_name', 'description'." You can then go ahead and check the value of each key with more confidence (and when checking values, I'd recommend comparing two dicts, again for the sake of having nice error messages).


@classmethod
def tearDownClass(cls):
"""Delete previously created repository.
"""
cls.repo.delete_repo()
cls.repo.last_response.raise_for_status()
Task.wait_for_tasks(cls.repo.last_response)


class RepoDeleteTestCase(TestCase):
"""Testing succesfull repo deletion.
"""

@classmethod
def setUpClass(cls):
"""Create repository and raise if not created.
"""
cls.repo = Repository(id=cls.__name__)
cls.repo.create_repo()
cls.repo.last_response.raise_for_status()
cls.repo.delete_repo()
cls.repo.get_repo()

def test_status_code(self):
"""Test if request on deleted repo returned 404, Not Found.
"""
self.assertEqual(self.repo.last_response.status_code, 404,
self.repo.last_response.json())

def test_body(self):
"""Test if body contains all data keys.
"""
self.assertLessEqual(
ERROR_KEYS,
set(self.repo.last_response.json().keys()),
self.repo.last_response.json())


class RepoUpdateTestCase(TestCase):
"""Create and then update repo. Test if updates were applied.
"""

@classmethod
def setUpClass(cls):
"""Create and update repo, get repo."""
cls.repo = Repository(id=cls.__name__)
cls.repo.create_repo()
cls.repo.last_response.raise_for_status()
delta = {
'display_name': cls.__name__,
'description': cls.__name__,
}
cls.repo.update_repo(delta)

def test_status_code(self):
"""Test that status code of update repo call is 200;
"""
self.assertEqual(self.repo.last_response.status_code, 200,
self.repo.last_response.json())

def test_body(self):
"""Test that repository description and display names are correct.
"""
self.assertTrue(all(self.repo.last_response.json()['result'][key] ==
self.__class__.__name__
for key in ['id', 'display_name', 'description']),
self.repo.last_response.json().keys())

@classmethod
def tearDownClass(cls):
"""Delete previously created repository.
"""
cls.repo.delete_repo()
cls.repo.last_response.raise_for_status()
Task.wait_for_tasks(cls.repo.last_response)