Code Review up to now

[jbenet]

Tested it out-- seems to work! 👍

• nice command help
• minor testing, seems to work well 👍 🎆 👏
• clear output tells the user what's going on
• nice reverting (with interactive select!) 😎
• nice ipfs-update versions output

Things to Fix

• command help could be more user friendly: no idea what to feed it here:

  ./ipfs-update revert --help
  NAME:
     ./ipfs-update revert - revert to previously installed version of ipfs
  
  USAGE:
     ./ipfs-update revert [arguments...]
  
  DESCRIPTION:
     revert will check if a previous update left a stashed
  binary and overwrite the current ipfs binary with it.
  

• use v<num> or <num>, be consistent:

  > ./ipfs-update versions
  v0.3.2
  v0.3.4
  v0.3.5
  v0.3.6
  v0.3.7
  v0.3.8
  v0.3.9
  > ./ipfs-update version
  0.3.9
  

• improve description of stash cmd, not clear what it would be used for.

• separate testnew.go into own subpkg -- https://github.com/ipfs/ipfs-update/blob/b4d672f8551bb17e8f84d19b35cfbf2b82a9f400/testnew.go

• separate the "update bundle" formatting parts into a subpkg (versions.go)

• use localApiUrl const for url

  ipfs-update/versions.go

  Line 31 in b4d672f

  sh := api.NewShell("http://localhost:5001")

• port is configurable...

  ipfs-update/versions.go

  Line 31 in b4d672f

  sh := api.NewShell("http://localhost:5001")

  • read $repo/api file (respect $IPFS_PATH)
  • respect --api flag

• should bound the httpFetch resp.Body reader with a limited reader (like at most {4MB, 8MB} or something) --

  ipfs-update/utils.go

  Line 24 in b4d672f

  mes, err := ioutil.ReadAll(resp.Body)

  to avoid hosing the local client's memory if there's a malicious remote host.

• Decompose into logical units!

  ipfs-update/install.go

  Line 17 in b4d672f

  func InstallVersion(root, v string, nocheck bool) error {

  -- 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.

• should say "error reverting" --

  ipfs-update/revert.go

  Line 20 in b4d672f

  stump.Log("error replacing binary after install fail: ", rnerr)

• Give user directions on what to do now:

  ipfs-update/revert.go

  Line 22 in b4d672f

  stump.Log("your old ipfs binary should still be located at: ", stashpath)

  -- maybe like

  maybe try: mv $stashpath $binpath
  

  Otherwise, this tool leaves users totally in the dust. "my old binary is in X, ok? what does that mean? what should i do?"

• mixing your stump thing with fmt.Fprintf !? --

  ipfs-update/revert.go

  Line 56 in b4d672f

  fmt.Fprintf(tw, "%d)\t%s\t%s\n", i+1, bin.Name(), bin.ModTime().Format(time.ANSIC))

  -- great now changing the logger will fail to catch these.

• if going to allow overriding stdio elsewhere, make this a variable reader too, not os.Stdio --

  ipfs-update/revert.go

  Line 61 in b4d672f

  scan := bufio.NewScanner(os.Stdin)

• repeat "(0 to exit)" here --

  ipfs-update/revert.go

  Line 68 in b4d672f

  stump.Log("please enter a number in the range 1-%d", len(entries))

• 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?

  ipfs-update version <version>, by <authors>
  

• 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.

• bad error -- handle errors better

  ./ipfs-update install 0.3.8
  installing ipfs version 0.3.8
  fetching 0.3.8 binary...
  ERROR: no link named "0.3.8" under QmctgsD3G5MrvUcuVAZ6tVXP2c2pkgsEsqT85gQaTcfGjw
  

• silently correct inputs not specifying the v (or clearly handle this error specially, by prompting the user to add it)

• none of this is tested

Overall, a good start! 👍

but needs lots of testing before i would be comfortable shipping this to users in a "this is the recommended upgrade path" state.

[jbenet]

any ideas on how to test this stuff? maybe a sharness suite that upgrades through all the versions runs tests on each version, does stuff and then verifies everything is still fine? like have some workloads (mix of adding + pinning/unpinning + gcing a lot of things) and then have the expected output, upgrade through and check everything is still the same. particularly important with the pinset changes for the migration. this could be a test for both migrations and ipfs-update that make sure everything is solid across version changes.

cc @chriscool thoughts?

[whyrusleeping]

mixing your stump thing with fmt.Fprintf !? --

ipfs-update/revert.go

Line 56 in b4d672f

fmt.Fprintf(tw, "%d)\t%s\t%s\n", i+1, bin.Name(), bin.ModTime().Format(time.ANSIC))

-- great now changing the logger will fail to catch these.

I'm trying to differentiate between 'output' and 'logs', Printf and friends are used for program output (i.e. ipfs-update version > myversion)

[whyrusleeping]

but needs lots of testing before i would be comfortable shipping this to users in a "this is the recommended upgrade path" state.

Fortunately this doesnt need to be a 'recommended' upgrade path until 0.4.0, we can ship 0.3.10 and still not "recommend" it as the primary upgrade path.

[jbenet]

@whyrusleeping ok sounds good. we'll need thorough testing by 0.4.0 though

[whyrusleeping]

@jbenet agreed. I'm hoping @chriscool has good ideas on testing this nicely, theres a good number of moving parts involved.

[chriscool]

Ok, I will have a look at testing it soon...

[whyrusleeping]

youre the bestest ever

[chriscool]

@whyrusleeping thanks!

Please take a look: #5