Skip to content
This repository has been archived by the owner on Feb 27, 2018. It is now read-only.

Add support for client binary on upgrade #242

Merged
merged 5 commits into from Dec 16, 2014

Conversation

nathanleclaire
Copy link
Contributor

Went ahead and implemented this, looking for feedback, how do those file permissions look to you?

ping @SvenDowideit @tianon

@SvenDowideit
Copy link
Contributor

mmm, a couple of things to consider
It'd be nice to only download if the version is different from the one you have.

On Linux, this is dangerous - I install my docker using the debs - so there it should probably (also?) be optional

@nathanleclaire
Copy link
Contributor Author

It'd be nice to only download if the version is different from the one you have.

Agreed, however I'm not sure what the best way to test this would be. Perhaps with the Github API?

On Linux, this is dangerous - I install my docker using the debs - so there it should probably (also?) be optional

Were you thinking this would be optional as a flag, or something that should prompt the user?

@SvenDowideit
Copy link
Contributor

yup, same GH api request as we make to get the iso version number (and we can amend that one to ask the VM what its version is too..)
I'd make it option based on a flag for now :)

@nathanleclaire
Copy link
Contributor Author

OK, I have added the requested changes:

  • only download client binary if user's version lags behind latest (non-RC) tag
  • skip client binary upgrade on Linux unless user manually sets --clobber flag

PTAL @SvenDowideit and @tianon 👍

url := "https://api.github.com/repos/docker/docker/tags"
tag, err := getLatestReleaseName(url)
if err != nil {
return fmt.Errorf("Error making Github API call to get docker client binary version: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalise Docker or you'll make James cry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;_;

@SvenDowideit
Copy link
Contributor

neat - do you want to do the same for the boot2docker binary? >:}

}
fmt.Printf("Success: downloaded %s\n\tto %s\n", binaryUrl, path)
} else {
fmt.Println("docker client is up to date, skipping client upgrade...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docker, and go on, tell them the version they have

@SvenDowideit
Copy link
Contributor

this would close boot2docker/osx-installer#63

@nathanleclaire
Copy link
Contributor Author

neat - do you want to do the same for the boot2docker binary? >:}

Yes - perhaps in a separate PR - this would over-write the existing (running) executable with the latest release if it is behind? And it would continue to be part of upgrade command, correct?

@nathanleclaire
Copy link
Contributor Author

OK @SvenDowideit I have implemented the additional feedback! Let me know if I missed anything. One thing I had thought of is - should we move existing docker binary to docker.bak or something instead of just steamrolling over it if there is a later version?

@@ -3,6 +3,7 @@ package main
import (
"encoding/json"
"fmt"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this blank line here for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an artifact from vim-go (run goimports on save hook).

@SvenDowideit
Copy link
Contributor

@nathanleclaire darn - i presume this got lost in the busyness?

@nathanleclaire
Copy link
Contributor Author

Yes - finishing it is relatively low hanging fruit but it's too late for this release I'm afraid :( (well, unless the b2d release goes later than the docker 1.3 release)

SvenDowideit pushed a commit to SvenDowideit/boot2docker-cli that referenced this pull request Nov 10, 2014
If mount fails, let's "modprobe" the partition type and try again
@nathanleclaire nathanleclaire force-pushed the upgrade_client_binary branch 2 times, most recently from e852ac8 to 46ea9a9 Compare December 13, 2014 04:11
@nathanleclaire
Copy link
Contributor Author

Implemented some new stuff (boot2docker-cli binary upgrade) and code review feedback. @SvenDowideit when you get back from vacation and @tianon PTAL

@@ -3,6 +3,9 @@ package main
import (
"encoding/json"
"fmt"
"net/http"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the random blank line here? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goimports as an editor hook >_>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this doesn't make sense as a goimports change. It's not "remote stuff" being pushed below, it's just other random stdlib imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must've accidentally added it - I'll fix

clientArch = "x86_64"
// todo: wishful thinking?
case "arm":
clientArch = "armv7l"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very wishful thinking, and will likely be very wrong (especially since it doesn't even account for GOARM, let alone the actual ARM version of the current system). Please remove and add a default: case that returns an error like was mentioned above. 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, this is where using uname -m as a URL component completely breaks down. For example, on my Pogoplug, I get armv5tel, but armv5el is also very much a thing (ie, the bits after the number mark supported features).

@tianon
Copy link
Contributor

tianon commented Dec 16, 2014

Lots of little nits, but overall I'm a fan. 👍

@SvenDowideit
Copy link
Contributor

I'm a big fan :)

`boot2docker upgrade` will support the Docker client binary,
boot2docker-cli binary, and boot2docker ISO now.

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
@nathanleclaire nathanleclaire force-pushed the upgrade_client_binary branch 2 times, most recently from 69e84dd to 11c898c Compare December 16, 2014 21:33
@nathanleclaire
Copy link
Contributor Author

Nice revised with the feedback from review. @SvenDowideit @tianon PTAL

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>

To do so, run the `boot2docker upgrade` command.

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add console here so we get nice highlights? 👼

ie:

$ command here
output here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely!

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
@tianon
Copy link
Contributor

tianon commented Dec 16, 2014

LGTM

1 similar comment
@daghack
Copy link
Contributor

daghack commented Dec 16, 2014

LGTM

tianon added a commit that referenced this pull request Dec 16, 2014
Add support for client binary on upgrade
@tianon tianon merged commit d2f3a3b into boot2docker:master Dec 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants