Skip to content

{Core} Extract User-Agent generation to get_az_user_agent#12734

Merged
jiasli merged 2 commits intoAzure:devfrom
jiasli:az-user-agent
Mar 25, 2020
Merged

{Core} Extract User-Agent generation to get_az_user_agent#12734
jiasli merged 2 commits intoAzure:devfrom
jiasli:az-user-agent

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Mar 24, 2020

Extract User-Agent generation to get_az_user_agent so that PR #12717 can also apply to az rest.

The logic to append custom User-Agent via AZURE_HTTP_USER_AGENT env var is removed from mgmt and data client factory, as this is already handled by msrest and causes duplication:

> $env:AZURE_HTTP_USER_AGENT='myua'
> az group list --debug
msrest.http_logger :     'User-Agent': 'python/3.8.2 (Windows-10-10.0.18362-SP0) msrest/0.6.1
1 myua msrest_azure/0.6.2 azure-mgmt-resource/8.0.1 Azure-SDK-For-Python AZURECLI/2.2.0 myua'

Note that myua appeared twice.

On the other hand, this logic is added to az rest to align with msrest. az rest previously can't handle AZURE_HTTP_USER_AGENT.


@mock.patch('azure.cli.core._profile.Profile.get_raw_token', autospec=True)
@mock.patch('requests.Session.send', autospec=True)
def test_send_raw_requests(self, send_mock, get_raw_token_mock):
Copy link
Member Author

Choose a reason for hiding this comment

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

These test methods are moved from the wrong class TestHandleException to the right class TestUtils.

Comment on lines +327 to +333
# Test custom case-insensitive User-Agent
with mock.patch.dict('os.environ', {'AZURE_HTTP_USER_AGENT': "env-ua"}):
send_raw_request(cli_ctx, 'GET', full_arm_rest_url, headers={'user-agent=ARG-UA'})

get_raw_token_mock.assert_called_with(mock.ANY, test_arm_active_directory_resource_id)
request = send_mock.call_args.args[1]
self.assertEqual(request.headers['User-Agent'], get_az_user_agent() + ' env-ua ARG-UA')
Copy link
Member Author

Choose a reason for hiding this comment

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

User-Agent for az rest is tested here.

@mock.patch('azure.cli.core.__version__', '7.8.9')
def test_get_az_user_agent(self):
actual = get_az_user_agent()
self.assertEqual(actual, 'AZURECLI/7.8.9')
Copy link
Member Author

@jiasli jiasli Mar 24, 2020

Choose a reason for hiding this comment

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

#12717 can use this test method to verify whether the installer is added to UA correctly.

@yonzhan yonzhan added this to the S167 milestone Mar 24, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 24, 2020

add to S167

@jiasli
Copy link
Member Author

jiasli commented Mar 24, 2020

Azure DevOps seems to be adding AZURE_HTTP_USER_AGENT=VSTS_0fb41ef4-5012-48a9-bf39-4ee3de03ee3 thus making the test fail:

FAIL: test_send_raw_requests (azure.cli.core.tests.test_util.TestUtils)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.6.10/x64/lib/python3.6/site-packages/mock/mock.py", line 1369, in patched
    return func(*newargs, **newkeywargs)
  File "/opt/hostedtoolcache/Python/3.6.10/x64/lib/python3.6/site-packages/azure/cli/core/tests/test_util.py", line 264, in test_send_raw_requests
    self.assertDictEqual(dict(request.headers), expected_header)
AssertionError: {'Use[38 chars]01937 VSTS_0fb41ef4-5012-48a9-bf39-4ee3de03ee3[208 chars]'12'} != {'Use[38 chars]01937', 'Accept-Encoding': 'gzip, deflate', 'A[154 chars]'12'}
  {'Accept': '*/*',
   'Accept-Encoding': 'gzip, deflate',
   'CommandName': 'rest',
   'Connection': 'keep-alive',
   'Content-Length': '12',
   'Content-Type': 'application/json',
   'ParameterSetName': 'method uri',
-  'User-Agent': 'AZURECLI/2.2.0.dev20200324101937 '
?                                                 -

+  'User-Agent': 'AZURECLI/2.2.0.dev20200324101937'}
?                                                  +

-                'VSTS_0fb41ef4-5012-48a9-bf39-4ee3de03ee35_build_246_0'}

Add @mock.patch.dict('os.environ') to manually remove it.

# if ENV_ADDITIONAL_USER_AGENT in os.environ:
# agents.append(os.environ[ENV_ADDITIONAL_USER_AGENT])

return ' '.join(agents)
Copy link
Contributor

Choose a reason for hiding this comment

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

May I know why we need the new function to handle agents? From my side, the previous UA definition should be fine, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because previously the functionality of concatenating UA is dispersed in 3 places:

  • mgmt client
  • data client
  • az rest

It is difficult to maintain. I extrated the logic to one place so that changes done here automatically applies to all 3 places, given #12717 is doing more tweaks on the UA.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

LGTM in general.

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.

4 participants