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

Accept any Iterable as tags parameter #542

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

bharel
Copy link

@bharel bharel commented Mar 4, 2020

Requirements for Contributing to this repository

  • Fill out the template below. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • The pull request must only fix one issue, or add one feature, at the time.
  • The pull request must update the test suite to demonstrate the changed functionality.
  • After you create the pull request, all status checks must be pass before a maintainer reviews your contribution. For more details, please see CONTRIBUTING.

What does this PR do?

Allow any Iterable to be the tags parameter.

Description of the Change

I made sure throughout the program, the tags parameter will be converted to a list.
This will allow you to enter any iterable, such as sets, and tuples to the tags parameter.

Alternate Designs

Thought of converting it to a list in each and every API function. Chose to do it in a central place.

Possible Drawbacks

None that I'm aware of.

Verification Process

Datadog test suite on CI/CD should run. Tests were updated accordingly.

Additional Notes

Nope.

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • 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 one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@bharel bharel requested a review from a team as a code owner March 4, 2020 12:11
jd
jd previously approved these changes Mar 4, 2020
@zippolyte
Copy link
Contributor

Hmm that still wouldn't work for metrics, events, ... sent through the API directly like api.Metric.send(...). Only lists are supported there still.
Do you think you could add that there too ? I'm don't want to merge this unless any iterables for tags are supported everywhere, so as not to cause confusion.

@bharel
Copy link
Author

bharel commented Mar 4, 2020

Seems to be much harder to add to API. Working on it.

@bharel
Copy link
Author

bharel commented Mar 4, 2020

@zippolyte The tests are non-deterministic for some reason. They sometimes pass and sometimes don't. Last 2 commits had some tests pass and some fail with no code difference.

@zippolyte
Copy link
Contributor

Hey @bharel,

The test issues are weird, I'm looking into them. Seems like an issue with how we recorded the API responses or something, will let you know

@zippolyte
Copy link
Contributor

Problem comes from the fact that in the test, when transforming into a list, the order of the tags array end up different than what it was when we recorded the API requests/responses, thus preventing a match.
Looking to see if we can ignore the array order in the request matchers we use.

@bharel
Copy link
Author

bharel commented Mar 5, 2020

Use-case wise, order shouldn't matter should it? Datadog doesn't seem to mind the order afaik.

@zippolyte
Copy link
Contributor

Yeah the order doesn't matter, it's just that we recorded the requests and responses in order to mock the real thing, and if the order changes in the replays, it won't match and the tests will fail.

I added a commit to your branch to make the the matcher ignore the order for tags arrays found in JSON body payloads. The tests seem to be passing now.

@zippolyte
Copy link
Contributor

/azp run DataDog.datadogpy.integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zippolyte
Copy link
Contributor

Okay, I think we're almost there. We just need to add something for the tags in metrics in the API client here: https://github.com/bharel/datadogpy/blob/50fc480f7e85ececefd99ff81d7f3825c0e8609b/datadog/api/metrics.py#L92-L103, as well as change lists to other iterables in the metrics and service checks tests and we should be good I believe

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days.
Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity.

@github-actions github-actions bot added the stale Stale - Bot reminder label Apr 10, 2020
@eliasdorneles
Copy link

Hello 👋
Any chance of reviving this?
Context: I'd love to be able to send set of tags instead of a list, would make it simpler to write tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale - Bot reminder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants