-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update request wrapper with password and A6 signature #5684
Conversation
@@ -24,10 +24,10 @@ | |||
|
|||
class CiscoACICheck(AgentCheck): | |||
|
|||
HTTP_CONFIG_REMAPPER = {'ssl_verify': {'name': 'tls_verify'}} | |||
HTTP_CONFIG_REMAPPER = {'ssl_verify': {'name': 'tls_verify'}, 'pwd': {'name': 'password'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you cover this in tests ?
Example:
integrations-core/etcd/tests/test_integration.py
Lines 162 to 192 in 1bf1bcf
@preview | |
@pytest.mark.parametrize( | |
'test_case, extra_config, expected_http_kwargs', | |
[ | |
("new auth config", {'username': 'new_foo', 'password': 'new_bar'}, {'auth': ('new_foo', 'new_bar')}), | |
("legacy ssl config True", {'ssl_verify': True}, {'verify': True}), | |
("legacy ssl config False", {'ssl_verify': False}, {'verify': False}), | |
("legacy ssl config unset", {}, {'verify': False}), | |
("timeout", {'prometheus_timeout': 100}, {'timeout': (100.0, 100.0)}), | |
], | |
) | |
def test_config(instance, test_case, extra_config, expected_http_kwargs): | |
instance.update(extra_config) | |
check = Etcd(CHECK_NAME, {}, [instance]) | |
with mock.patch('datadog_checks.base.utils.http.requests') as r: | |
r.get.return_value = mock.MagicMock(status_code=200) | |
check.check(instance) | |
http_kwargs = dict( | |
auth=mock.ANY, | |
cert=mock.ANY, | |
data=mock.ANY, | |
headers=mock.ANY, | |
proxies=mock.ANY, | |
timeout=mock.ANY, | |
verify=mock.ANY, | |
) | |
http_kwargs.update(expected_http_kwargs) | |
r.post.assert_called_with(URL + '/v3alpha/maintenance/status', **http_kwargs) |
And maybe add a test for existing ssl_verify
remapping :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Christine's PTO, we'll get this in and add tests during QA
What does this PR do?
Cisco_aci uses the request wrapper but uses the older agent signature. Also add
pwd
to remapping.Motivation
Additional Notes
Review checklist (to be filled by reviewers)
changelog/
andintegration/
labels attached