Skip to content
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

feat: add user-agent to templates #507

Merged
merged 46 commits into from
May 10, 2023
Merged

feat: add user-agent to templates #507

merged 46 commits into from
May 10, 2023

Conversation

everly-gif
Copy link
Contributor

add user-agent to templates

@lohanidamodar
Copy link
Member

@everly-gif are you working on these?

@everly-gif everly-gif marked this pull request as ready for review March 11, 2023 11:53
@lohanidamodar
Copy link
Member

@stnguyen90 I think the SDK version makes sense in the user-agent.

@@ -43,6 +43,7 @@ class ClientIO extends ClientBase with ClientMixin {
'x-sdk-platform': '{{ sdk.platform }}',
'x-sdk-language': '{{ language.name | caseLower }}',
'x-sdk-version': '{{ sdk.version }}',
'user-agent' : '{{spec.title | caseUcfirst}}{{ language.name | caseUcfirst }}SDK/({{Platform.operatingSystem}}; {{Platform.operatingSystemVersion}})',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing sdk version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -65,6 +65,7 @@ func NewClient() Client {
headers := map[string]string{
{% for key,header in spec.global.defaultHeaders %}
"{{key}}" : "{{header}}",
"user-agent" : "{{spec.title | caseUcfirst}}{{ language.name | caseUcfirst }}SDK/{{ sdk.version }}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In go, you can use runtime.GOOS:

package main

import (
	"fmt"
	"runtime"
)

func main() {
	s := fmt.Sprintf("(%s;%s)", runtime.GOOS, runtime.GOARCH)
	fmt.Println(s)
}

outputs:

(linux;amd64)

https://go.dev/play/p/aS0Ik4UnKXj

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -11,6 +12,7 @@ class Client {
this.headers = {
'accept-encoding': '*',
'content-type': '',
'user-agent' : `{{spec.title | caseUcfirst}}{{language.name | caseUcFirst}}SDK/{{ sdk.version }} (${os.type()} ${os.version()}; ${os.arch()})`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the ; be after the os type rather than after the version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

})
}),
new TwigFilter('deviceInfo', function ($value) {
return php_uname('s') . ';' . php_uname('v') . ';' . php_uname('m');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PHP, every value is only separated by ; and no spaces. This isn't consistent between the various user agents.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is just os name, type and arch separated by ; same as other SDKs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other SDKs have it separated by ; (extra space) rather than just ;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it updated

@lohanidamodar lohanidamodar requested review from stnguyen90 and abnegate and removed request for christyjacob4 and Meldiron May 2, 2023 06:10
@abnegate
Copy link
Contributor

abnegate commented May 2, 2023

@lohanidamodar Do we want to update the client SDKs user-agent header format? They will be inconsistent with these otherwise

@lohanidamodar
Copy link
Member

@lohanidamodar Do we want to update the client SDKs user-agent header format? They will be inconsistent with these otherwise

Only difference will be, in the client we want to use the package info of the app instead of SDK/version right. That's intended. For server side SDKs apps package, version is no much of importance

})
}),
new TwigFilter('deviceInfo', function ($value) {
return php_uname('s') . ';' . php_uname('v') . ';' . php_uname('m');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other SDKs have it separated by ; (extra space) rather than just ;

@abnegate
Copy link
Contributor

abnegate commented May 3, 2023

@lohanidamodar Do we want to update the client SDKs user-agent header format? They will be inconsistent with these otherwise

Only difference will be, in the client we want to use the package info of the app instead of SDK/version right. That's intended. For server side SDKs apps package, version is no much of importance

True makes sense 👌

templates/node/lib/client.js.twig Outdated Show resolved Hide resolved
templates/go/client.go.twig Show resolved Hide resolved
lohanidamodar and others added 2 commits May 3, 2023 13:42
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
@lohanidamodar lohanidamodar self-requested a review May 10, 2023 07:11
})
}),
new TwigFilter('deviceInfo', function ($value) {
return php_uname('s') . ';' . php_uname('v') . ';' . php_uname('m');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it updated

@lohanidamodar lohanidamodar merged commit 7ea4e37 into master May 10, 2023
2 checks passed
@lohanidamodar lohanidamodar deleted the feat-add-user-agent branch May 10, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants