Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Checking for updates #31

Closed
wants to merge 12 commits into from
Closed

Checking for updates #31

wants to merge 12 commits into from

Conversation

dborzov
Copy link
Contributor

@dborzov dborzov commented Aug 18, 2014

This PR makes ipfs reach the Github API endpoint for the list of this repo's git tags on every run. It then attempts to read the tag names as semver version labels and compares them to the running one (defined as a Version const literal in cmd/ipfs/version.go).

If the running version is older than the most recent available, the error message is shown (suggesting strongly to update) and the program exits.

This behaviour can be changed with updates.check flag in the config file. ignore and warn values are supported.

Please also note the related issue.

@dborzov dborzov changed the title Checking for updates on start Checking for updates Aug 18, 2014
@jbenet jbenet force-pushed the master branch 2 times, most recently from ecf49fe to 2a7dafd Compare August 21, 2014 03:44
@jbenet
Copy link
Member

jbenet commented Sep 12, 2014

@dborzov oh wow, i somehow completely missed this PR. sorry @dborzov ! would you mind rebasing it on top of new master? then we can use it

@dborzov
Copy link
Contributor Author

dborzov commented Sep 12, 2014

LOL, sure, I will do it this weekend.

@jbenet
Copy link
Member

jbenet commented Sep 16, 2014

thanks @dborzov :)

also, @inconshreveable made this: https://github.com/inconshreveable/go-update which could be pretty useful. I think though that for that sort of update, it should:

  • ask the user whether they want to update
  • should check for signatures on releases (not sure best way to do this with standard go-tool style installations...)

@inconshreveable
Copy link

@jbenet go-update has built-in support for signing and validating releases

@jbenet
Copy link
Member

jbenet commented Sep 16, 2014

@inconshreveable oh great! i somehow missed that, thanks 👍

@dborzov
Copy link
Contributor Author

dborzov commented Oct 6, 2014

Hey, so I am halfway through the rebase. The key changes:

  • Add Updates fields to the config: updates.check (can be 'error', 'warn' or 'ignore') and updates.version (version with which the ipfs node was initialized)
  • Add CheckForUpdates() error function (I put it into cmd/ipfs/version.go, it probably does not belong there, anyone has better ideas?) that makes a Github API request for releases in this repo and compares tag's semvers to the one set up in cmd/ipfs/version.go as a literal and returns the corresponding error.
  • For now I added the checker only to the local node initializer ( link). I am a little bit fuzzy on how you guys set up all that daemon business just yet, will need to read up stuff more.

@dborzov
Copy link
Contributor Author

dborzov commented Oct 6, 2014

Will try to use go-update for the actual updating. Maybe in a separate pull-request?

@whyrusleeping
Copy link
Member

I think the best place for any code relating to the updating (CheckForUpdates()) should live in its own package, maybe in the root as "updates" or in "config/updates"

@jbenet
Copy link
Member

jbenet commented Oct 7, 2014

@dborzov all this sgtm.

For now I added the checker only to the local node initializer. I am a little bit fuzzy on how you guys set up all that daemon business just yet, will need to read up stuff more.

This is a fine place for now. Later on it can be in different common location for the cmd/ipfs tool.

I think the best place for any code relating to the updating (CheckForUpdates()) should live in its own package, maybe in the root as "updates" or in "config/updates"

Agreed. i'd go for updates.

@@ -129,6 +129,12 @@ func initCmd(c *commander.Command, inp []string) error {
},
}

// tracking ipfs version used to generate the init folder and adding update checker default setting.
cfg.Updates = config.Updates{
Check: "error",
Copy link
Member

Choose a reason for hiding this comment

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

maybe these values ("error", "ignore", etc) should be constants in the update package:

const CheckError = "error"
const CheckIgnore = "ignore"
...

@dborzov
Copy link
Contributor Author

dborzov commented Oct 7, 2014

@whyrusleeping , @jbenet thanks for the feedback and CR. I updated the thing with all the changes you suggested.

@dborzov
Copy link
Contributor Author

dborzov commented Oct 7, 2014

Being able to actually update go-ipfs by running ipfs version update using https://github.com/inconshreveable/go-update will be a different PR then.

@@ -58,7 +60,8 @@ Use "ipfs help <command>" for more information about a command.
func init() {
config, err := config.PathRoot()
if err != nil {
config = ""
u.POut("Failure initializing the default Config Directory: ", err)
os.Exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

@dborzov this should probably not fail if the -c flag is provided to a run, which we don't know about here. I'll leave this in for now, until the -c flag works throughout commands. (doesn't atm i think)

// Version regulates checking if the most recent version is run
type Version struct {
Check string `json:"check"` // "ignore" for do not check, "warn" and "error" for reacting when obsolete
Current string `json:"current"` // ipfs version for which config was generated
Copy link
Member

Choose a reason for hiding this comment

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

should we lowercase all config var names? the capitalized json is certainly odd-looking, but more admittedly more readable

cc @dborzov @whyrusleeping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove the lower case tags for now for consistency.

Not to bikeshed over this too much, but the obvious future for config files format is probably toml and we will need to adopt it at some point IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

i love all cases equally, i dont discriminate.

Copy link
Member

Choose a reason for hiding this comment

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

Also, you dont have to specify the lower case tag names, if your struct field is "Test" and in json its "test", go figures it out just fine.

@jbenet
Copy link
Member

jbenet commented Oct 9, 2014

@dborzov sorry for extra comments after you thought we were all set! but i think these will help the UX of the updates.

@jbenet jbenet mentioned this pull request Oct 9, 2014
* master: (107 commits)
  bugfixes to prev commit
  u.DOut -> log.Debug
  fixed resolver test
  ipfs name cmd improvements
  Skip ipns_test.TestMultiWrite in darwin
  skip ipns fuse tests in travis
  add another test to try and reproduce data loss issue
  add in some extra debug logging, and increase routing table latencies
  comment out debug msg
  use encoded (pretty) keys only on fs ds
  make vendor is your friend
  keytransform ds
  Rework package structure for unixfs and subpackage
  update error handling in ipns
  add more comments!
  vendor things
  New NameSystem interface
  deprecate merkledag.Node.Update
  ipns TestFastRepublish
  init SetupLoggers
  ...

Conflicts resolved:
	cmd/ipfs/gen.go
	cmd/ipfs/ipfs.go
	config/config.go
@dborzov
Copy link
Contributor Author

dborzov commented Oct 9, 2014

@jbenet ok, added these changes too

@jbenet
Copy link
Member

jbenet commented Oct 9, 2014

@dborzov ok, everything LGTM! thank you! I'll handle this merge later today, as tests failing, etc.

@jbenet
Copy link
Member

jbenet commented Oct 10, 2014

Merged in 16b5345

@jbenet jbenet closed this Oct 10, 2014
@jbenet
Copy link
Member

jbenet commented Oct 10, 2014

Thank you @dborzov!

This was referenced Oct 14, 2014
longfeiWan9 pushed a commit to longfeiWan9/go-ipfs that referenced this pull request Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants