{"action":"created","issue":{"url":"https://api.github.com/repos/ipfs/ipfs-update/issues/4","labels_url":"https://api.github.com/repos/ipfs/ipfs-update/issues/4/labels{/name}","comments_url":"https://api.github.com/repos/ipfs/ipfs-update/issues/4/comments","events_url":"https://api.github.com/repos/ipfs/ipfs-update/issues/4/events","html_url":"https://github.com/ipfs/ipfs-update/issues/4","id":119469445,"number":4,"title":"Code Review up to now","user":{"login":"jbenet","id":138401,"avatar_url":"https://avatars.githubusercontent.com/u/138401?v=3","gravatar_id":"","url":"https://api.github.com/users/jbenet","html_url":"https://github.com/jbenet","followers_url":"https://api.github.com/users/jbenet/followers","following_url":"https://api.github.com/users/jbenet/following{/other_user}","gists_url":"https://api.github.com/users/jbenet/gists{/gist_id}","starred_url":"https://api.github.com/users/jbenet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jbenet/subscriptions","organizations_url":"https://api.github.com/users/jbenet/orgs","repos_url":"https://api.github.com/users/jbenet/repos","events_url":"https://api.github.com/users/jbenet/events{/privacy}","received_events_url":"https://api.github.com/users/jbenet/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-11-30T11:01:37Z","updated_at":"2015-12-04T07:31:56Z","closed_at":null,"body":"## Tested it out-- seems to work! :+1: \r\n\r\n- nice command help\r\n- minor testing, seems to work well :+1:  :fireworks: :clap:\r\n- clear output tells the user what's going on\r\n- nice reverting ([with interactive select!](https://38.media.tumblr.com/947ceaa2cef127f0481b388f8d9a9551/tumblr_mvq95tg9Kj1snx6rjo2_400.gif)) :sunglasses: \r\n- nice `ipfs-update versions` output\r\n\r\n![](https://ipfs.io/ipfs/QmRfndgFK4XWHus1yBwbpzpsuYSLftCsJ2KptYPrMf4LTj/shia-clap.gif)\r\n![](http://www.gifbin.com/bin/012014/1391200591_daft_punk_grammy_clap.gif)\r\n\r\n## Things to Fix\r\n\r\n- [x] command help could be more user friendly: no idea what to feed it here:\r\n\r\n  ```\r\n  ./ipfs-update revert --help\r\n  NAME:\r\n     ./ipfs-update revert - revert to previously installed version of ipfs\r\n\r\n  USAGE:\r\n     ./ipfs-update revert [arguments...]\r\n\r\n  DESCRIPTION:\r\n     revert will check if a previous update left a stashed\r\n  binary and overwrite the current ipfs binary with it.\r\n  ```\r\n\r\n- [x] use `v<num>` or `<num>`, be consistent:\r\n\r\n    ```\r\n    > ./ipfs-update versions\r\n    v0.3.2\r\n    v0.3.4\r\n    v0.3.5\r\n    v0.3.6\r\n    v0.3.7\r\n    v0.3.8\r\n    v0.3.9\r\n    > ./ipfs-update version\r\n    0.3.9\r\n    ```\r\n\r\n- [x] improve description of `stash` cmd, not clear what it would be used for.\r\n- [x] separate `testnew.go` into own subpkg -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/testnew.go\r\n- [ ] separate the \"update bundle\" formatting parts into a subpkg (versions.go)\r\n- [x] use `localApiUrl` const for url https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/versions.go#L31\r\n- [ ] port is configurable... https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/versions.go#L31\r\n  - [ ] read `$repo/api` file (respect $IPFS_PATH)\r\n  - [ ] respect `--api` flag\r\n- [ ] should bound the `httpFetch` `resp.Body` reader with a limited reader (like at most {4MB, 8MB} or something) -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/utils.go#L24 to avoid hosing the local client's memory if there's a malicious remote host.\r\n- [ ] Decompose into logical units! https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/install.go#L17 -- long sequences of tangential instructions ARE NOT easy to follow. I had to break it up into this: https://gist.github.com/jbenet/aa51fce4fb271f92070e just to check everything made sense. Use that or something even better.\r\n- [ ] should say \"error reverting\" -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/revert.go#L20\r\n- [ ] Give user directions on what to do now: https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/revert.go#L22 -- maybe like\r\n\r\n  ```\r\n  maybe try: mv $stashpath $binpath\r\n  ```\r\n  Otherwise, this tool leaves users totally in the dust. \"my old binary is in X, ok? what does that mean? what should i do?\"\r\n- [ ] mixing your stump thing with `fmt.Fprintf` !? -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/revert.go#L56 -- great now changing the logger will fail to catch these.\r\n- [ ] if going to allow overriding stdio elsewhere, make this a variable reader too, not os.Stdio -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/revert.go#L61\r\n- [ ] repeat `\"(0 to exit)\"` here -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/revert.go#L68\r\n\r\n- [ ] can the \"VERSION\" and \"AUTHORS\" parts be removed from standard `--help` output? i think it's distracting to the average user. or if not, then maybe as one subtle line at the end? \r\n\r\n    ```\r\n    ipfs-update version <version>, by <authors>\r\n    ```\r\n\r\n- [ ] The habit of writing out to the user from the bowels of the code is very bad. It makes me never want to touch this code, as any meaningful refactor could have unintended serious impact in the flow of the UX.\r\n\r\n\r\n- [ ] bad error -- handle errors better\r\n\r\n  ```\r\n  ./ipfs-update install 0.3.8\r\n  installing ipfs version 0.3.8\r\n  fetching 0.3.8 binary...\r\n  ERROR: no link named \"0.3.8\" under QmctgsD3G5MrvUcuVAZ6tVXP2c2pkgsEsqT85gQaTcfGjw\r\n  ```\r\n\r\n- [ ] silently correct inputs not specifying the `v` (or clearly handle this error specially, by prompting the user to add it)\r\n\r\n- [ ] [none of this is tested](http://i.imgur.com/EPK2WvB.jpg)\r\n\r\n### Overall, a good start! :+1: \r\n\r\n#### but needs lots of testing before i would be comfortable shipping this to users in a \"this is the recommended upgrade path\" state."},"comment":{"url":"https://api.github.com/repos/ipfs/ipfs-update/issues/comments/161898508","html_url":"https://github.com/ipfs/ipfs-update/issues/4#issuecomment-161898508","issue_url":"https://api.github.com/repos/ipfs/ipfs-update/issues/4","id":161898508,"user":{"login":"whyrusleeping","id":1243164,"avatar_url":"https://avatars.githubusercontent.com/u/1243164?v=3","gravatar_id":"","url":"https://api.github.com/users/whyrusleeping","html_url":"https://github.com/whyrusleeping","followers_url":"https://api.github.com/users/whyrusleeping/followers","following_url":"https://api.github.com/users/whyrusleeping/following{/other_user}","gists_url":"https://api.github.com/users/whyrusleeping/gists{/gist_id}","starred_url":"https://api.github.com/users/whyrusleeping/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/whyrusleeping/subscriptions","organizations_url":"https://api.github.com/users/whyrusleeping/orgs","repos_url":"https://api.github.com/users/whyrusleeping/repos","events_url":"https://api.github.com/users/whyrusleeping/events{/privacy}","received_events_url":"https://api.github.com/users/whyrusleeping/received_events","type":"User","site_admin":false},"created_at":"2015-12-04T07:31:56Z","updated_at":"2015-12-04T07:31:56Z","body":"youre the bestest ever"}}