-
Notifications
You must be signed in to change notification settings - Fork 74
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
Update check #208
Update check #208
Conversation
I will look into merging the goreleaser template changes first. That would allow me to do more testing. |
internal/cmd/update.go
Outdated
/* updateCmd.AddCommand(&cobra.Command{ | ||
Use: "check", | ||
Hidden: true, | ||
Short: "Check if there are any updates available", | ||
RunE: func(_ *cobra.Command, _ []string) error { | ||
return updateCLI("", false) | ||
}, | ||
}) | ||
*/ | ||
/* updateCmd.AddCommand(&cobra.Command{ | ||
Use: "install", | ||
Hidden: true, | ||
Short: "Update the tool to the latest version", | ||
PersistentPreRun: func(_ *cobra.Command, _ []string) { | ||
opts.cfg.SkipUpdateCheck = true | ||
}, | ||
PreRun: func(cmd *cobra.Command, args []string) { | ||
opts.args = args | ||
}, | ||
RunE: func(_ *cobra.Command, _ []string) error { | ||
return updateCLI(opts) | ||
}, | ||
}) | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what are these?? Future state?? If the plan is to have it be nancy update check
and nancy update install
we should probably first pass make it actually be nancy update check
instead of changing the api again probably very soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what are these?? Future state?? If the plan is to have it be
nancy update check
andnancy update install
we should probably first pass make it actually benancy update check
instead of changing the api again probably very soon.
Yeah, far future state. The really nice CircleCI CLI code I'm basing this off of has some complex (to my brain anyway) logic that sets some flags (opts...
) on "hidden" commands to affect if/when an "install" is attempted (dryRun
). I don't think I want to go down the same path. Maybe I'll change my mind if a light bulb goes on for me, but I'm gonna try and keep it simpler.
I kept these commented chunks around mostly to remember to flesh out the "install" stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return err | ||
} | ||
|
||
if update.ShouldCheckForUpdates(updateCheck) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine....but thinking of how we do CI.... not much gets cached. So once we make this automatic its going to be checking all the time if the underlying docker CI containers always get blown away. We we still want to do it automatically we probably need to add something to the README about caching the ~/.ossindex dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine....but thinking of how we do CI.... not much gets cached. So once we make this automatic its going to be checking all the time if the underlying docker CI containers always get blown away. We we still want to do it automatically we probably need to add something to the README about caching the ~/.ossindex dir.
Great point! I'll try to add some doco about this. I actually like the idea of it printing messages about an updated Nancy version in CI builds. Slightly increasing the chance a human will notice it in the CI logs. Still worth mentioning though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some docs here: c62fded
internal/cmd/update.go
Outdated
|
||
func logAndShowMessage(message string) { | ||
logLady.Info(message) | ||
fmt.Println(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to respect the quiet flag here??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe its fine.... realistically this is for humans only right?? Not really a CI thing and probably doesn't hurt to log some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your thinking, and am still leaning to leaving the "nag" enabled even in "quiet" mode. If somebody complains, we can tweak things.
// You can override this value using -X flag to the compiler ldflags. This is | ||
// overridden when we build for Homebrew. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this still needs done so that homebrew
will be set here??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this still needs done so that
homebrew
will be set here??
Err, um, yes? This falls into the bucket of "I still don't know exactly how that will all work". :)
As per @bhamail ... this is some sample output |
Fixed a boog in the autocheck execution path. Some sample output:
|
…to track future work.
Automatic check for new releases. Prompt user when new version is available.
Automatic check for new releases. Prompt user when new version is available.
The automatic check will only occur once every 28 hours.
The timestamp of the lookup is stored in an app specific yml file:
~/.ossindex/.nancy-config/update_check.yml
This PR also adds an explicit
update
command that can be used to force a check for a new release. e.g.nancy update
In order for the release lookup logic to be happy, I had to change the goreleaser release filename template a little. (see
.goreleaser.yml
)The related logic in the selfupdater is: https://github.com/rhysd/go-github-selfupdate/blob/master/selfupdate/detect.go#L95. The suffixes used to find a matching release file didn't like the 'version' on the tail end, so I moved it.
Note: I'm guessing something in homebrew-nancy-tap will need to change to match the new filename pattern (or maybe it'll "just work")?
Once we have some releases with the new filename pattern, I can work on adding "auto update" stuff for certain cases.
It relates to the following issue #s:
cc @bhamail / @DarthHater