{"action":"created","issue":{"url":"https://api.github.com/repos/SatelliteQE/nailgun/issues/41","labels_url":"https://api.github.com/repos/SatelliteQE/nailgun/issues/41/labels{/name}","comments_url":"https://api.github.com/repos/SatelliteQE/nailgun/issues/41/comments","events_url":"https://api.github.com/repos/SatelliteQE/nailgun/issues/41/events","html_url":"https://github.com/SatelliteQE/nailgun/issues/41","id":68979431,"number":41,"title":"Review entity methods","user":{"login":"Ichimonji10","id":1012989,"avatar_url":"https://avatars.githubusercontent.com/u/1012989?v=3","gravatar_id":"","url":"https://api.github.com/users/Ichimonji10","html_url":"https://github.com/Ichimonji10","followers_url":"https://api.github.com/users/Ichimonji10/followers","following_url":"https://api.github.com/users/Ichimonji10/following{/other_user}","gists_url":"https://api.github.com/users/Ichimonji10/gists{/gist_id}","starred_url":"https://api.github.com/users/Ichimonji10/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Ichimonji10/subscriptions","organizations_url":"https://api.github.com/users/Ichimonji10/orgs","repos_url":"https://api.github.com/users/Ichimonji10/repos","events_url":"https://api.github.com/users/Ichimonji10/events{/privacy}","received_events_url":"https://api.github.com/users/Ichimonji10/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/SatelliteQE/nailgun/labels/breaking%20change","name":"breaking change","color":"f7c6c7"}],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/SatelliteQE/nailgun/milestones/5","html_url":"https://github.com/SatelliteQE/nailgun/milestones/Make%20%22beta%22%20quality","labels_url":"https://api.github.com/repos/SatelliteQE/nailgun/milestones/5/labels","id":1191195,"number":5,"title":"Make \"beta\" quality","description":"NailGun is currently listed as being \"pre-alpha\" quality. I'd like to push NailGun forward so that it is more viable than that. I'd like to make NailGun high-quality enough that it can be used by third-parties without hesitation. What are the qualities of a high-quality product?\r\n\r\n* Well documented.\r\n* Well tested.\r\n* Stable enough to be forward-compatible with future changes.\r\n* Provides significant utility and is easy to use.\r\n\r\nThis milestone is much like the \"alpha\" milestone. However, there are some differences:\r\n\r\n* The various entity methods should finally be normalized. (#41) This is an interesting problem that may require several tries to get right, but *some* good solution should be adopted. If an even better solution makes an appearance in the future, then that's a big enough improvement that it would rightfully warrant a major version bump (e.g. from 1.4.2 to 2.0.0).\r\n* Fix other issues that will affect NailGun's API. For example, #135 will definitely affect the API.","creator":{"login":"Ichimonji10","id":1012989,"avatar_url":"https://avatars.githubusercontent.com/u/1012989?v=3","gravatar_id":"","url":"https://api.github.com/users/Ichimonji10","html_url":"https://github.com/Ichimonji10","followers_url":"https://api.github.com/users/Ichimonji10/followers","following_url":"https://api.github.com/users/Ichimonji10/following{/other_user}","gists_url":"https://api.github.com/users/Ichimonji10/gists{/gist_id}","starred_url":"https://api.github.com/users/Ichimonji10/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Ichimonji10/subscriptions","organizations_url":"https://api.github.com/users/Ichimonji10/orgs","repos_url":"https://api.github.com/users/Ichimonji10/repos","events_url":"https://api.github.com/users/Ichimonji10/events{/privacy}","received_events_url":"https://api.github.com/users/Ichimonji10/received_events","type":"User","site_admin":false},"open_issues":1,"closed_issues":1,"state":"open","created_at":"2015-07-02T16:55:38Z","updated_at":"2015-07-24T13:21:09Z","due_on":null,"closed_at":null},"comments":28,"created_at":"2015-04-16T18:10:32Z","updated_at":"2015-08-04T13:44:36Z","closed_at":null,"body":"Several methods in module `nailgun.entities` do not map nicely to an underlying path. <ins>In addition, several methods do not have appropriate arguments.</ins> The methods exposed by the entity classes should be reviewed to make sure that they are appropriately named and placed on the most appropriate class.\r\n\r\nMethods to review:\r\n\r\n* [x] `ActivationKey.add_subscriptions`\r\n* [x] `ActivationKey.content_override`\r\n* [x] `AbstractDockerContainer.power`\r\n* [x] `AbstractDockerContainer.logs`\r\n* [x] `ContentViewVersion.promote`\r\n* [x] `ContentView.publish`\r\n* [x] `ContentView.set_repository_ids`\r\n* [x] ~~`ContentView.available_puppet_modules`~~\r\n* [x] `ContentView.add_puppet_module`\r\n* [x] `ContentView.copy`\r\n* [x] ~~`ForemanTask.poll`~~\r\n* [x] `Organization.subscriptions`\r\n* [x] `Organization.upload_manifest`\r\n* [x] `Organization.delete_manifest`\r\n* [x] `Organization.refresh_manifest`\r\n* [x] `Organization.sync_plan`\r\n* [x] ~~`Organization.list_rhproducts`~~ (EntitySearchMixin is used instead)\r\n* [x] ~~`Permission.search`~~ (EntitySearchMixin is used instead)\r\n* [x] `Product.list_repositorysets`\r\n* [x] ~~`Product.fetch_rhproduct_id`~~ (EntitySearchMixin is used instead)\r\n* [x] ~~`Product.fetch_reposet_id`~~ (EntitySearchMixin is used instead)\r\n* [x] `Product.enable_rhrepo`\r\n* [x] `Product.disable_rhrepo`\r\n* [x] `Repository.sync`\r\n* [x] ~~`Repository.fetch_repoid`~~ (EntitySearchMixin is used instead)\r\n* [x] `Repository.upload_content`\r\n* [x] `RHCIDeployment.add_hypervisors`\r\n* [x] `RHCIDeployment.deploy`\r\n* [x] `SyncPlan.add_products`\r\n* [x] `SyncPlan.remove_products`\r\n\r\nOne example of an entity method that does not map nicely to an underlying path is [`Organization.upload_manifest`](http://nailgun.readthedocs.org/en/latest/api/nailgun.html#nailgun.entities.Organization.upload_manifest). This method POSTs to the following path:\r\n\r\n    /katello/api/v2/organizations/<organization_id>/subscriptions/upload\r\n\r\nHowever, the API documentation lists a second API path that can also be POSTed to:\r\n\r\n    /katello/api/v2/subscriptions/upload\r\n\r\nThe latter approach is preferred. The method `Organization.upload_manifest` is not clearly related to any underlying path, but the method `Subscription.upload` is. There are also a bevy of problems with nested paths: they make it hard to determine what the canonical location of a resource it, it promotes a proliferation of the size of the API, and it makes it hard to manage special-case logic. (Special case logic is more easily managed when building a JSON payload than when building a URL.) Thus, `Organization.upload_manifest` should be dropped in favor of `Subscription.upload`. It could look like this:\r\n\r\n```python\r\ndef upload(\r\n        self, content, organization_id, repository_url=None,\r\n        synchronous=True):\r\n    \"\"\"Upload a subscription manifest file.\r\n\r\n    :param content: A file handle to a manifest file. In other words,\r\n        the object returned by `open('path/to/file', 'rb')`.\r\n    :param int organization_id: An organization ID.\r\n    :param str repository_url: An optional repository URL.\r\n    :param bool synchronous: What should happen if the server returns an\r\n        HTTP 202 (accepted) status code? Wait for the task to complete if\r\n        ``True``. Immediately return JSON response otherwise.\r\n    :returns: The server's response, with all JSON decoded.\r\n    :rtype: dict\r\n    :raises: ``nailgun.entity_mixins.TaskTimedOutError`` if an HTTP 202\r\n        response is received, ``synchronous is True`` and the task\r\n        completes with any result other than \"success\".\r\n\r\n    \"\"\"\r\n    data = {'organization_id': organization_id}\r\n    if repository_url is not None:\r\n        data['repository_url'] = repository_url\r\n    response = client.post(\r\n        self.path('upload'),\r\n        data,\r\n        auth=self._server_config.auth,\r\n        files={'content': content},\r\n        verify=self._server_config.verify,\r\n    )\r\n    response.raise_for_status()\r\n\r\n    # Poll a task if necessary, then return the JSON response.\r\n    if synchronous is True and response.status_code == httplib.ACCEPTED:\r\n        return ForemanTask(\r\n            self._server_config,\r\n            id=response.json()['id']\r\n        ).poll()\r\n    return response.json()\r\n```"},"comment":{"url":"https://api.github.com/repos/SatelliteQE/nailgun/issues/comments/127617718","html_url":"https://github.com/SatelliteQE/nailgun/issues/41#issuecomment-127617718","issue_url":"https://api.github.com/repos/SatelliteQE/nailgun/issues/41","id":127617718,"user":{"login":"Ichimonji10","id":1012989,"avatar_url":"https://avatars.githubusercontent.com/u/1012989?v=3","gravatar_id":"","url":"https://api.github.com/users/Ichimonji10","html_url":"https://github.com/Ichimonji10","followers_url":"https://api.github.com/users/Ichimonji10/followers","following_url":"https://api.github.com/users/Ichimonji10/following{/other_user}","gists_url":"https://api.github.com/users/Ichimonji10/gists{/gist_id}","starred_url":"https://api.github.com/users/Ichimonji10/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Ichimonji10/subscriptions","organizations_url":"https://api.github.com/users/Ichimonji10/orgs","repos_url":"https://api.github.com/users/Ichimonji10/repos","events_url":"https://api.github.com/users/Ichimonji10/events{/privacy}","received_events_url":"https://api.github.com/users/Ichimonji10/received_events","type":"User","site_admin":false},"created_at":"2015-08-04T13:44:36Z","updated_at":"2015-08-04T13:44:36Z","body":"@oshtaier That's basically it. We have to do some gymnastics on the back end to make everything work nicely. But the end result is that a user gets to work with a \"Subscription\" entity without worrying about which of the several API paths are used for a given call. On the back end, everything should work pretty nicely going forward, too. Let's say that Satellite deprecates all the long paths in favor of short paths, or deprecates all the short paths in favor of long paths. That's no problem - we can use a little versioning logic to deal with the change and the user is never the wiser."}}