Adding api for repositories and tasks and few repository tests

[peterlacko]

No description provided.

[Ichimonji10]

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

[Ichimonji10]

I see that this pull request has been submitted from the master branch. You may wish to learn about using Git branching and merging. Using branches allows you to work on multiple tasks at once. You can add some repository tests in one branch, task tests in another branch, experiment with some helpers in a third branch, and so on. Super useful. See: http://www.git-scm.com/book/en/v2/Git-Branching-Basic-Branching-and-Merging

I'm also going to request that you squash the commits in this pull request before it is merged. The end result is that this pull request should have very few commits - probably one. This makes it easier to review the commit log and revert changes if necessary. If you want to learn how to do a squash, try researching the topic, and ask me if you need further help.

[Ichimonji10]

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?

[peterlacko]

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

[Ichimonji10]

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

[peterlacko]

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.

[Ichimonji10]

Ahhh, now I understand. Thanks for explaining. 😄

[Ichimonji10]

I appreciate what you've done with creating the Repository and Task helper classes, @peterlacko. However, I'd really like to hold off on adding that functionality to Pulp Smash. Adding more tests and then writing up helper classes will give us more use cases, which helps to ensure that whatever we do implement will be more likely to stick. (Also, my own experiences writing API bindings for Satellite 6 inform my judgment call here.)

In the meantime, feel free to add something simple like pulp_smash/constants.py and pulp_smash/helpers.py, where the two modules have objects like REPOSITORY_PATH and poll_task(), respectively. I expect that these two modules will change or be dropped entirely as time goes on. But they're simple things, and should be fairly easy to manipulate when the time comes.

[peterlacko]

@Ichimonji10: Thank you for thoughtful and detailed responses, I'm refining my code now.

Ad: Helper classes: I didn't write them with intention to stay there for long time/forever, but only as simple temporary solution which can be easily extended or replaced later when we will have more use cases for them. I think that for now they could help us to write tests, ie. create and delete repository by on simple method call.

Ad: Branching and committing: I will follow your recommendations ;-)

[Ichimonji10]

Ad: Helper classes: I didn't write them with intention to stay there for long time/forever, but only as simple temporary solution which can be easily extended or replaced later when we will have more use cases for them. I think that for now they could help us to write tests, ie. create and delete repository by on simple method call.

Aye, I understand the intent behind writing them. But in my opinion, they don't quite qualify as "simple". To use or understand code that makes use of Repository, a user must answer questions like:

• When a Repository object is instantiated, what happens to the arguments passed in? Are they saved as instance attributes? (No. They're saved in the data_keys attribute.)
• Do all arguments correspond to values accepted by the server, or are some keywords reserved by the object itself? (All args are passed to the server.)
• Do the methods like create_repo and delete_repo mutate the object they're called on? (Yes.) If so, how?
• Do methods like create_repo and delete_repo return anything? (No.)
• Why is it that __init__ and create accept an arbitrary set of kwargs, but update_repo explicitly constrains what values can be passed in via its signature? (I don't know.)
• The methods include "repo" in their name. Does this mean that I have to learn a new set of method names for every single class? (Yes.) Does this mean each class is unique in other ways? (Yes.)

So please, do not add helper classes right now. A simple set of constants (and maybe functions) is enough.

Mind you, I'm expecting unit tests for any globally accessible code. Code has a nasty habit of sticking around longer than expected.

[peterlacko]

I'm expecting unit tests for any globally accessible code

What do you mean by it?

[Ichimonji10]

What do you mean by it?

If a private function is added to a test module, no unit tests need to be added. For example, if you add a function called _handle_response() to pulp_smash.tests.platform.api_v2.test_repository.py, no unit test needs to be added for that function.

On the other hand, if a module like pulp_smash.resources.platform.api_v2 is added, then unit tests should be added for that new module.

[omaciel]

Talked to @peterlacko and he agreed that we should close this PR. He will submit another soon.

[Ichimonji10]

Thank you.

FYI, #28 was just merged, and https://github.com/Ichimonji10/pulp-smash/tree/zoo_repo_sync is in the works.