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.

Sign up for GitHub

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

Jump to bottom

Add RequestsWrapper option to support UTF-8 for basic auth #7441

Merged
merged 2 commits into from
Aug 28, 2020
Merged

Add RequestsWrapper option to support UTF-8 for basic auth #7441

merged 2 commits into from
Aug 28, 2020

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Aug 27, 2020 •
edited
Loading

Motivation

Part 1 of solution to #7176

@ofek ofek requested review from a team as code owners August 27, 2020 17:30
@codecov
Copy link

codecov bot commented Aug 27, 2020 •
edited
Loading

Codecov Report

Merging #7441 into master will decrease coverage by 0.63%.
The diff coverage is n/a.

Impacted Files Coverage Δ
disk/tests/common.py 64.28% <0.00%> (-35.72%) ⬇️
disk/tests/conftest.py 85.71% <0.00%> (-11.43%) ⬇️
disk/datadog_checks/disk/disk.py 83.66% <0.00%> (-3.59%) ⬇️
disk/tests/test_filter.py 96.45% <0.00%> (-3.55%) ⬇️
gitlab/tests/conftest.py
ignite/datadog_checks/ignite/__init__.py
kubelet/datadog_checks/kubelet/__about__.py
nginx/tests/test_vts.py
tomcat/datadog_checks/tomcat/__about__.py
mapr/datadog_checks/mapr/mapr.py
... and 740 more

@FlorianVeaux
Copy link
Member

This is quite a big change for our integrations which could cause issues.
If a server only supports iso-8859-1/latin1 and not utf8, our integrations will suddenly stop working.
The question of "how many servers" are used that only support latin-1 is a difficult one.

We also know from issue #7176 that RabbitMQ only supports utf-8
Also utf-8 has been the default in browser for some time: https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#Character_encoding_of_HTTP_authentication

ChristineTChen
ChristineTChen previously approved these changes Aug 28, 2020
View reviewed changes
@ofek ofek changed the title Ensure RequestsWrapper supports unicode characters for basic auth Add RequestsWrapper option to support UTF-8 for basic auth Aug 28, 2020
@ofek ofek requested review from a team as code owners August 28, 2020 16:27
@ghost ghost added the dev/tooling label Aug 28, 2020
ChristineTChen
ChristineTChen previously approved these changes Aug 28, 2020
View reviewed changes
@ofek ofek merged commit a666c42 into master Aug 28, 2020
@ofek ofek deleted the ofek/h branch August 28, 2020 19:54
github-actions bot pushed a commit that referenced this pull request Aug 28, 2020
@ofek
Add RequestsWrapper option to support UTF-8 for basic auth (#7441)
d777543 
* Add RequestsWrapper option to support UTF-8 for basic auth

* Update test_load.py a666c42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristineTChen ChristineTChen ChristineTChen approved these changes

Assignees
No one assigned
Labels
dev/tooling integration/activemq_xml integration/airflow integration/amazon_msk integration/ambari integration/apache integration/cilium integration/cisco_aci integration/cloud_foundry_api integration/cockroachdb integration/consul integration/couch integration/couchbase integration/datadog_checks_base integration/datadog_checks_dev integration/druid integration/elastic integration/envoy integration/etcd integration/fluentd integration/gitlab_runner integration/gitlab integration/hdfs_datanode integration/hdfs_namenode integration/http_check integration/istio integration/kong integration/kube_apiserver_metrics integration/lighttpd integration/mapreduce integration/marathon integration/mesos_master integration/mesos_slave integration/nginx_ingress_controller integration/nginx integration/openmetrics integration/php_fpm integration/rabbitmq integration/riak integration/spark integration/squid integration/twistlock integration/vault integration/yarn
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ofek @FlorianVeaux @ChristineTChen @coignetp