[teleport-update] Reuse MakeURL moved to common package #51383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vapopov
force-pushed
the
vapopov/client-tools-update-uri-template-agent
branch
3 times, most recently
from
0efad1d to
b9a36d8
Compare
vapopov
added
the
no-changelog
sclevine
reviewed
Jan 22, 2025
vapopov
force-pushed
the
vapopov/client-tools-update-uri-template-agent
branch
from
b9a36d8 to
721915e
Compare
sclevine
approved these changes
Jan 23, 2025
|
@sclevine ah sure, we have to add base url env for agent as well |
|
@sclevine I've replaced with base url by setting it from config/flag/env, but kept in |
Base automatically changed from
vapopov/client-tools-update-uri-template
to
master
January 24, 2025 21:47
MakeURL moved to common function to be general for both, agent and client tools
vapopov
force-pushed
the
vapopov/client-tools-update-uri-template-agent
branch
from
b03dfad to
a26978f
Compare
sclevine
reviewed
Jan 25, 2025
| return trace.Errorf("Teleport download URL must use TLS (https://)") | ||
| if spec.BaseURL != "" && | ||
| !strings.HasPrefix(strings.ToLower(spec.BaseURL), "https://") { | ||
| return trace.Errorf("Teleport download URL must use TLS (https://): %q", spec.BaseURL) |
There was a problem hiding this comment.
Suggested change
| return trace.Errorf("Teleport download URL must use TLS (https://): %q", spec.BaseURL) | |
| return trace.Errorf("Teleport download base URL %s must use TLS (https://)", spec.BaseURL) |
Quotes are escaped with structured logging and don't look great
| @@ -127,15 +126,15 @@ func (li *LocalInstaller) Remove(ctx context.Context, rev Revision) error { | |||
| // Install a Teleport version directory in InstallDir. | |||
| // This function is idempotent. | |||
| // See Installer interface for additional specs. | |||
| func (li *LocalInstaller) Install(ctx context.Context, rev Revision, template string) (err error) { | |||
| func (li *LocalInstaller) Install(ctx context.Context, rev Revision, template string, baseURL string) (err error) { | |||
There was a problem hiding this comment.
Can we remove the template param and move to it to a Template field on LocalInstaller? It could be set to autoupdate.DefaultCDNURITemplate in NewLocalUpdater, and directly set in tests.
There was a problem hiding this comment.
Make sense, I kept this one only for test in such case
| @@ -185,7 +184,7 @@ type Updater struct { | |||
| type Installer interface { | |||
| // Install the Teleport agent at revision from the download template. | |||
| // Install must be idempotent. | |||
| Install(ctx context.Context, rev Revision, template string) error | |||
| Install(ctx context.Context, rev Revision, template string, baseURL string) error | |||
There was a problem hiding this comment.
Suggested change
| Install(ctx context.Context, rev Revision, template string, baseURL string) error | |
| Install(ctx context.Context, rev Revision, baseURL string) error |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactoring after moving
MakeURLandRevisionto common packageDepends on: #51210