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

Add space_id and org_id tags in addition to space_guid and org_guid #7382

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

bkabrda
Copy link
Contributor

@bkabrda bkabrda commented Aug 17, 2020

What does this PR do?

Adds space_id and org_id tags in addition to space_guid and org_guid to make sure users can use the same set of tags as they use for the datadog-firehose-nozzle.

Motivation

We should provide usage consistency with https://github.com/DataDog/datadog-firehose-nozzle which submits org_id and space_id tags.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@@ -405,15 +405,22 @@ def build_dd_event(
org_guid,
):
# type: (str, str, int, str, str, str, str, str, str, str, str) -> Event
space_id = space_guid if space_guid else "none"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this leave space_guid to be None instead of the string "none"?
Should this be:

space_id = space_guid = space_guid if space_guid else "none"

(Same as org_id below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

space_id is used as a value for both space_id: and space_guid: tags - and this is exactly what was used for space_guid previously, so I don't see any issue (or I'm missing something :)).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, my bad. I missed that it was using the same variable for the value. LGTM

@bkabrda bkabrda merged commit 3a500e7 into master Aug 20, 2020
@bkabrda bkabrda deleted the slavek.kabrda/cfapi-add-id-tags branch August 20, 2020 08:24
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.

3 participants