{"action":"created","issue":{"url":"https://api.github.com/repos/docker/machine/issues/286","labels_url":"https://api.github.com/repos/docker/machine/issues/286/labels{/name}","comments_url":"https://api.github.com/repos/docker/machine/issues/286/comments","events_url":"https://api.github.com/repos/docker/machine/issues/286/events","html_url":"https://github.com/docker/machine/pull/286","id":54469953,"number":286,"title":"WIP: Refactor code out of main package","user":{"login":"cjellick","id":2473240,"avatar_url":"https://avatars.githubusercontent.com/u/2473240?v=3","gravatar_id":"","url":"https://api.github.com/users/cjellick","html_url":"https://github.com/cjellick","followers_url":"https://api.github.com/users/cjellick/followers","following_url":"https://api.github.com/users/cjellick/following{/other_user}","gists_url":"https://api.github.com/users/cjellick/gists{/gist_id}","starred_url":"https://api.github.com/users/cjellick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cjellick/subscriptions","organizations_url":"https://api.github.com/users/cjellick/orgs","repos_url":"https://api.github.com/users/cjellick/repos","events_url":"https://api.github.com/users/cjellick/events{/privacy}","received_events_url":"https://api.github.com/users/cjellick/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-01-15T16:28:02Z","updated_at":"2015-01-21T14:16:18Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/docker/machine/pulls/286","html_url":"https://github.com/docker/machine/pull/286","diff_url":"https://github.com/docker/machine/pull/286.diff","patch_url":"https://github.com/docker/machine/pull/286.patch"},"body":"This PR does the following:\r\n* Moves the ```store.go``` and ```host.go``` code out of the main package and into a package called ```store```\r\n* Introduces an ```api``` package to provide a single entry point for all operations on hosts\r\n\r\n## Motivation and benefits\r\nFirst, hopefully this will better organize business logic and separate concerns in the machine code base. It should give future developers better guidance on where business logic should go and force them to think through how it can be cleanly exposed. This should make machine more maintainable in the long run. \r\n\r\nIt should allow ```commands.go``` to be concerned only with interpreting CLI requests and passing them to the API.  With the code in it's current state, it isn't clear where new functionality should drop. Here's a few examples where it seems the current model is falling short:\r\n* We've seen some PRs where important store-specific logic is dropped directly into ```commands.go`` #29 is a possible example of this.\r\n* The way one performs state changing operations is inconsistent. For example, to start a host you call ```host.Start()```, to restart it you call ```host.DriverRestart()```, and to remove it you call ```store.Remove()```, but there is also a ```host.Remove()``` function exposed.\r\n\r\nSecond, this provides access to machine as a library so that integrators don't have to exec out to it. This is a good stop-gap measure while we work towards building a REST API for machine. \r\n\r\nAs @shykes and @ehazlett mentioned in an IRC conversation, a risk is that introducing a public api at this point means being forced to maintain it. It should be understood that the ```api``` provides no stability or backwards compatibility guarantees with regards to public consumption. It is first and foremost an internal API for better separating concerns within the machine code base. Users calling machine as a library would do so at their own risk. Would calling this out explicitly as a comment in the ```api ``` package be sufficient for conveying this?\r\n\r\n## Current deficencies or concerns\r\nThere a few deficencies with the PR as-is:\r\n\r\nThe SSH command code has note been refactored. This was just done for time savings. I wanted to get some feedback on the concept before spending the time to refactor it.\r\n\r\nThe abstraction is a little leaky. All state changing functions on the ```host``` struct have been unexposed, but  the ```Driver``` field is still exposed and it exposes state changing operations. The reason for this is that the ```machine inspect``` command marshals the Driver field to json. I couldn't simply hide the Driver field because it would cause inspect to be much less useful. Suggests on how to address this are welcome.\r\n\r\nThere is some logic in ```store.go``` and ```host.go``` that perhaps now should be moved directly into the ```api``` package. This was done intentionally to minimize the impact of the PR. Future PRs could be address further refactoring made possible by this PR.\r\n\r\nSome more testing may be needed"},"comment":{"url":"https://api.github.com/repos/docker/machine/issues/comments/70843887","html_url":"https://github.com/docker/machine/pull/286#issuecomment-70843887","issue_url":"https://api.github.com/repos/docker/machine/issues/286","id":70843887,"user":{"login":"sthulb","id":103217,"avatar_url":"https://avatars.githubusercontent.com/u/103217?v=3","gravatar_id":"","url":"https://api.github.com/users/sthulb","html_url":"https://github.com/sthulb","followers_url":"https://api.github.com/users/sthulb/followers","following_url":"https://api.github.com/users/sthulb/following{/other_user}","gists_url":"https://api.github.com/users/sthulb/gists{/gist_id}","starred_url":"https://api.github.com/users/sthulb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sthulb/subscriptions","organizations_url":"https://api.github.com/users/sthulb/orgs","repos_url":"https://api.github.com/users/sthulb/repos","events_url":"https://api.github.com/users/sthulb/events{/privacy}","received_events_url":"https://api.github.com/users/sthulb/received_events","type":"User","site_admin":false},"created_at":"2015-01-21T14:16:18Z","updated_at":"2015-01-21T14:16:18Z","body":"The `api` package should be reserved for when we offer a HTTP API perhaps"}}