Add OS info to lakectl UA
#7759
Conversation
eladlachmi
added
area/lakectl
E2E Test Results - DynamoDB Local - Local Block Adapter |
.gitallowed
Outdated
| @@ -6,3 +6,4 @@ AKIAJZZZZZZZZZZZZZZQ | |||
| AKIAIOSFDNN7EXAMPLEQ | |||
| wJalrXUtnFEMI/K3MDENG/bPxRfiCYEXAMPLEKEY | |||
| wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY | |||
| AKIAIOSFOLQUICKSTART | |||
There was a problem hiding this comment.
I think it was due to your experiments with the test server. Try to remove it and see if you see the error again
There was a problem hiding this comment.
Can you try removing it and see if it fails?
There was a problem hiding this comment.
Interesting... I removed it, and the commit wasn't blocked. I wonder if the next commit will be blocked, and maybe that's how this got committed in the first place 😅
| client, err := apigen.NewClientWithResponses( | ||
| serverEndpoint, | ||
| apigen.WithHTTPClient(httpClient), | ||
| apigen.WithRequestEditorFn(basicAuthProvider.Intercept), | ||
| apigen.WithRequestEditorFn(func(ctx context.Context, req *http.Request) error { | ||
| req.Header.Set("User-Agent", "lakectl/"+version.Version) | ||
| req.Header.Set("User-Agent", fmt.Sprintf("lakectl/%s/%s/%s/%s", version.Version, oss.OS, oss.Version, oss.Platform)) |
There was a problem hiding this comment.
Please add a comment here. Someone can naively change this and not understand the implications.
Generally, as we already discussed I'm not on favor of this approach 🙂
| oss := OSInfo{ | ||
| OS: "windows", | ||
| Version: ver, | ||
| Platform: "unknown", |
There was a problem hiding this comment.
You can also get the os architecture in windows
There was a problem hiding this comment.
I looked around for a bit and didn't find a reliable way of doing this. If you know of one, please let me know.
There was a problem hiding this comment.
wmic cpu get DataWidth is not good enough?
There was a problem hiding this comment.
I don't think it is. In terms of 32/64bit, modern Windows won't even run on 32bit processors. The more interesting part is x86 vs. ARM, and that command won't give you that. Once windows on ARM becomes more of a thing, there will probably be a better way of doing this. Right now, I don't see much value in this information TBH.
cmd/lakectl/cmd/root.go
Outdated
| @@ -462,12 +463,13 @@ func getClient() *apigen.ClientWithResponses { | |||
| DieErr(err) | |||
| } | |||
|
|
|||
| oss, _ := osinfo.GetOSInfo() | |||
There was a problem hiding this comment.
Why are we returning an error if we're not using it?
There was a problem hiding this comment.
Removed the error and added a default return value to all implementations
pkg/osinfo/osinfo_darwin.go
Outdated
| "time" | ||
| ) | ||
|
|
||
| func GetOSInfo() (OSInfo, error) { |
There was a problem hiding this comment.
In all the OSInfo variations, you already know the OS type. Suggest to remove the error and always return some info
There was a problem hiding this comment.
Good point. Done.
|
|
||
| func GetOSInfo() OSInfo { | ||
| out, err := getInfo() | ||
| for strings.Contains(out, brokenPipeOutput) { |
|
|
||
| func GetOSInfo() OSInfo { | ||
| out, err := getInfo() | ||
| for strings.Contains(out, brokenPipeOutput) { |
| out, err = getInfo() | ||
| time.Sleep(retryWaitTime) | ||
| } | ||
| if err != nil { |
| out, err = getInfo() | ||
| time.Sleep(retryWaitTime) | ||
| } | ||
| if err != nil { |
pkg/osinfo/osinfo_darwin.go
Outdated
| osInfo := strings.Split(osStr, " ") | ||
| oss := OSInfo{ | ||
| OS: osInfo[0], | ||
| Version: osInfo[1], | ||
| Platform: osInfo[2], | ||
| } |
There was a problem hiding this comment.
Comment for all code instances - this could easily panic and requires handling in case slice len != 3
Closes #7712
Change Description
Background
This PR adds basic OS information to the
lakectlUser-Agentstring. In aggregate, this information can support decisions regarding the development oflakectl.New Feature
The following properties are added to the UA string used by
lakectl:Testing Details
Tested manually + unit test included in this PR.