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

Encode hostname in set_external_tags #3866

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

therve
Copy link
Contributor

@therve therve commented Jun 5, 2019

The go code is expecting bytes, let's make sure that's what we send.

The go code is expecting bytes, let's make sure that's what we've sent.
@@ -333,10 +333,12 @@ def set_external_tags(self, external_tags):
# ('hostname2', {'src2_name': ['test2:t3']})
# ]
try:
for _, source_map in external_tags:
new_tags = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Didnt @masci factor this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're waiting for 6.12 to release to merge it IIUC.

@dcoleman17 dcoleman17 requested a review from hush-hush June 5, 2019 12:26
@therve therve merged commit 8a4c542 into master Jun 5, 2019
@therve therve deleted the therve/external-tags-unicode-hosts branch June 5, 2019 16:51
ofek pushed a commit that referenced this pull request Jun 5, 2019
The go code is expecting bytes, let's make sure that's what we send.
ofek added a commit that referenced this pull request Jun 21, 2019
ofek pushed a commit that referenced this pull request Jun 21, 2019
* remove duplicated code

* fix property signature

* reuse more code with to_string

* change property into constant

* use plain empty string

* move 'normalize' to the base class

* style fix

* moved docs over from datadog-agent

* avoid deleting the class object so Sphinx can parse it

* style fix

* add .base. to the module path

* update from #3866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants