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

Fix user agent string #22454

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Fix user agent string #22454

merged 1 commit into from
Jun 23, 2021

Conversation

srnagar
Copy link
Member

@srnagar srnagar commented Jun 22, 2021

This PR fixes the user agent string to include the name and version of the SDK used. The update here is to set the artifact-id in autorest configuration that generates the azure-monitor-query.properties and update the builders to read the values from the properties file.

@srnagar srnagar requested a review from pallavit as a code owner June 22, 2021 20:45
@srnagar srnagar enabled auto-merge (squash) June 23, 2021 19:01
@@ -22,6 +22,7 @@ required-fields-as-ctor-args: true
model-override-setter-from-superclass: true
credential-types: tokencredential
client-side-validations: true
artifact-id: azure-monitor-query
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a new addition to the autorest. How were the version and SDK being loaded until this was done. I don't think I did anything special for ACR but I do have the usual builder plumbing for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like ACR is missing azure-containers-containerregistry.properties file. Adding the above to your autorest config should generate this file and also update the builder to read from this file. This was manually done in the past, but now we have support for this in autorest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I will create a TODO for myself :)

@pallavit
Copy link
Contributor

pallavit commented Jun 23, 2021

        "/.default");

Also, for the token credential it may be worth exposing this as an overridable method that adds policy - in autorest, So that the builders can be used to create the rest of the pipeline but still extend the auth policy.


Refers to: sdk/monitor/azure-monitor-query/src/main/java/com/azure/monitor/query/metricsnamespaces/implementation/MetricsNamespacesClientImplBuilder.java:240 in 8685024. [](commit_id = 8685024, deletion_comment = False)

Copy link
Contributor

@pallavit pallavit left a comment

Choose a reason for hiding this comment

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

:shipit:

@srnagar srnagar merged commit cf85dfc into Azure:main Jun 23, 2021
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.

2 participants