-
Notifications
You must be signed in to change notification settings - Fork 17
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
[DT-4] Auto-update feature #74
Conversation
Pull Request Test Coverage Report for Build 951
💛 - Coveralls |
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.
Great start! I have a few comments for you
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 looks good to me!
How about you @jocgir ?
Wait up. Let's review Linux and Windows independently, please. The other PR also should've been merged independently. Essentially, you shouldn't bunch up 3 PRs worth of stuff in a single one. I will merge the update script PR into master but you need to split up linux and Windows stuff |
3d2b514
to
1cbcb91
Compare
Here I merged the linux script update. To be clear, open: I think there will be enough discussion to warrant three PRs |
1cc76ac
to
2246942
Compare
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.
Sad trombone
I'm getting
environment: line 33: /home/jduchesne/repos/tgf/tgf: Text file busy
We may have to do the Windows trick even on linux
@@ -208,5 +217,21 @@ func (app *TGFApplication) Run() int { | |||
Printf("tgf v%s\n", version) | |||
return 0 | |||
} | |||
|
|||
if app.DeleteCopy { | |||
os.Remove(getHomeFileName("tgf")) |
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.
give it a hidden name. not tgf
} | ||
|
||
if app.CopyPath != "" { | ||
cloneExecutableAt(app.CopyPath) |
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 should delete the copy if it exists
func getCurrentExecutablePath() string { | ||
executablePath, err := os.Executable() | ||
if err != nil { | ||
printWarning("Error getting executable path", err) |
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.
If there's an error. This will return ""
and then the next command will fail
"path/filepath" | ||
) | ||
|
||
// RunUpdate runs the update on the current tgf executable |
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 deserves to be way better documented. An outsider has to be able to figure out what will happen with this doc
cmd.Run() | ||
} | ||
|
||
func cloneExecutableAt(newExecutablePath string) string { |
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 used in another file. It shouldn't be private and it should be documented
var dueForUpdate = lastRefresh(autoUpdateFileName) > 2*time.Hour | ||
if app.DoUpdate && dueForUpdate && RunUpdate() { | ||
touchImageRefresh(autoUpdateFileName) | ||
ReRunCopy() |
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'd call that ForwardCallToNewVersion
. It's long but descriptive
@@ -10,8 +10,12 @@ import ( | |||
) | |||
|
|||
func getTouchFilename(image string) string { | |||
return getHomeFileName(util.EncodeBase64Sha1(image)) |
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.
There's still a whole bunch of duplicated logic and no clear distinction where the code should go (this file or update_*.go). I suggest putting all homedir handling to a file called homedir.go
. You could then have simple commands that only need the filename as parameter:
Read
Write (This can replace touch)
Done:
no-update
flag to skip the auto updateTODO: