Interactively configure lakeFS server URL on first lakectl login
#9659
Conversation
- OpenAPI support - Login tokens abstraction - Controller hookup to login tokens abstraction (This feature is unimplemented in base lakeFS, and only a trivial login tokens abstraction exists here.)
This is typically only called by the browser -- but it's still handled as OpenAPI in the controller.
Use the same RetryClient type as the rest of lakeFS, only with a different retry policy - one that retries status code 404. This involves refactoring getClient... so do that.
releaseToken is _not_ part of the UI, and there is no implicit redirection there from middleware. Instead, redirect there from the controller.
There was a problem hiding this comment.
Pull Request Overview
This PR implements interactive configuration of the lakeFS server URL on first lakectl login, addressing issue #9658. When the server endpoint URL is not configured, users are now prompted to enter it interactively, and the configuration is automatically saved.
Key changes:
- Removed the default server endpoint URL (
http://127.0.0.1:8000) from configuration initialization - Added interactive prompt with URL validation to collect server URL from users during first login
- Automatically saves the provided server URL to the configuration file
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/lakectl/cmd/root.go | Removed default server endpoint URL to enable interactive configuration |
| cmd/lakectl/cmd/login.go | Added URL validation, interactive prompt, and configuration saving logic for server endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return "", err | ||
| } | ||
| if serverURL.Path == "" { | ||
| serverURL.Path += "/api/v1" |
There was a problem hiding this comment.
The condition checks if Path is empty, but then uses += to append. When Path is empty, += will still work correctly, but this should use = instead of += for clarity and correctness. If Path were empty, \"\" + \"/api/v1\" equals \"/api/v1\", but the use of += suggests appending to an existing value, which is misleading.
| serverURL.Path += "/api/v1" | |
| serverURL.Path = "/api/v1" |
| if serverURL.Path == "" { | ||
| serverURL.Path += "/api/v1" | ||
| } | ||
| return serverURL.String(), err |
There was a problem hiding this comment.
The function returns err which is always nil at this point (from the previous url.Parse call that succeeded due to the comment on line 73). This should return nil explicitly instead of err to avoid confusion and potential future bugs if the code changes.
| return serverURL.String(), err | |
| return serverURL.String(), nil |
arielshaqed
force-pushed
the
feature/1194-lakectl-login-swagger
branch
from
bdb4f20 to
58a254b
Compare
Isan-Rivkin
left a comment
Isan-Rivkin
left a comment
There was a problem hiding this comment.
Will continue this review after the base PR's are ready again
2 participants
Closes #9658 .