-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cmd/tailscale/cli: make "update" work on Windows #6996
Conversation
return "", err | ||
} | ||
defer f.Close() | ||
f2, err := os.CreateTemp("", "tailscale-updater-*.exe") |
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 seems like this might trigger Windows Defender, writing out an executable and running it.
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.
Dunno?
It didn't for me. I don't think I have any of the relevant things turned off. Doesn't look like it.
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.
Windows Defender is kind of a black box wrt this, hard to say. This is a very common thing for updaters to do, though.
1d01d3c
to
513968c
Compare
513968c
to
663ae70
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.
This LGTM to me. This is an area where the rest of y'all know way more than I do, I'll leave it to you.
Okay, waiting on @dblohm7 to do another pass over it. I'll resume work on Debian support. |
@@ -24,7 +37,7 @@ import ( | |||
var updateCmd = &ffcli.Command{ | |||
Name: "update", | |||
ShortUsage: "update", | |||
ShortHelp: "Update Tailscale to the latest/different version", | |||
ShortHelp: "[ALPHA] Update Tailscale to the latest/different version", |
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.
Have we used "Alpha" or other warnings in other parts of the CLI? Not afaik?
If not, then not sure we should include, unless we're going to add throughout the CLI.
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.
Yes, we have. I made it match the others:
bradfitz@tsdev:~/src/tailscale.com$ git grep ALPHA
cmd/tailscale/cli/login.go: ShortUsage: "[ALPHA] login [flags]",
cmd/tailscale/cli/serve.go: ShortHelp: "[ALPHA] Serve from your Tailscale node",
cmd/tailscale/cli/serve.go:*** ALPHA; all of this is subject to change ***
cmd/tailscale/cli/switch.go: [ALPHA] switch <name>
cmd/tailscale/cli/switch.go: [ALPHA] switch --list
cmd/tailscale/cli/update.go: ShortHelp: "[ALPHA] Update Tailscale to the latest/different version",
663ae70
to
5c976e9
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.
Only concern is ensuring the correct ACL on %ProgramData%\Tailscale
, so I would like to see one more rev.
732ea44
to
24c50d3
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.
SHIP IT
24c50d3
to
7db362d
Compare
Updates #6995 Co-authored-by: Aaron Klotz <aaron@tailscale.com> Change-Id: I16622f43156a70b6fbc8205239fd489d7378d57b Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
7db362d
to
43ab0b0
Compare
cmd/tailscale/cli: make "update" work on Windows
Updates #6995