-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Client tools auto update logic for multiple platforms #46587
Conversation
974e35d
to
433f481
Compare
|
||
return archive, archive + ".sha256", nil | ||
} | ||
|
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 put together a more productionized version of these methods for the agent version, which adds disk space checking, timeouts, context cancellation, etc.: https://github.com/gravitational/teleport/compare/sclevine/teleport-update-enable
We could try to reuse those, but the use cases are a bit different. For now, you might want to look at some of the differences. E.g., use of the client and contexts to handle graceful cancellation, and limiting the number of bytes read over the network.
Add request timeout and default client Feature flag check
6e7505c
to
9e215fd
Compare
return requestedVersion, true | ||
} | ||
|
||
return toolsVersion, false |
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.
@russjones If the user downloads newer version of tsh/tctl and let say execute it from download directory, should we alway re-execute with saved version from tools directory (where version might be older)
@@ -0,0 +1,26 @@ | |||
//go:build webassets_ent |
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.
Would be better to introduce new build tag for enterprise version instead of reusing this one for web assets?
@vapopov - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
|
||
switch runtime.GOOS { | ||
case "darwin": | ||
archive = baseUrl + "/tsh-" + toolsVersion + ".pkg" |
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 one should be replaced with "/teleport" + toolsVersion + ".pkg"
only one issue we currently have is that teleport-{version}.pkg
has tsh
and tctl
binary without support of the TouchID for tsh
, so we need to install tsh-{version}.pkg
additionally, this one is valid only for the <v16, for the >v17 we should have everything in one teleport-{version}.pkg
package
Would be better to have separate packages for all platforms with the tsh
and tctl
only to minimize the download size for the client autoupdate.
cc @russjones
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.
2k lines is a bit intense to review, I'll definitely miss some stuff here.
Would you mind splitting the PR in smaller chunks? I think at least the webclient, common.update and tctl changes can be separated, but feel free to split using any other criteria.
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.
tool
's content should be limited to bootstrapping the CLI and commands. Complex logic should live in lib/
.
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.
ok, will try to split the logic, but it actually interconnected. Locking might be delivered separately, also integration tests kind of heavy. Webclient btw temporally added until this PR is merged
Closing in prior to split logic in series of PRs:
|
In this PR implemented logic for client tools autoupdate {tsh, tctl}, added integration tests for simulations of downloading, unpacking and replacing tools binaries from test CDN server (pre-combiled binary which contains only separated updater logic).
changelog: Added auto update logic for tsh, tctl client tools