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

Move Agent stubs to ddev (Part 1/2) #6171

Closed
wants to merge 10 commits into from
Closed

Move Agent stubs to ddev (Part 1/2) #6171

wants to merge 10 commits into from

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Mar 27, 2020 •
edited
Loading

What does this PR do?

Move stubs to dev.

This change has been divided into 2 PRs in order to preserve git history.

  • Part 1: Move files
  • Part 2: Modify files

If we merged both at the same time, the git history will be lost.

CI Results of the 2 PRs: #6176

Motivation

Stubs are mean to be used for testing and should not be part of the base package, but should be in dev package instead.

Needed for #6027 (comment)

Related discussion on why the stubs are in base package in the first place: #3700 (comment)

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

@codecov
Copy link

codecov bot commented Mar 27, 2020 •
edited
Loading

Codecov Report

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

Impacted Files Coverage Δ
http_check/datadog_checks/http_check/utils.py
ambari/tests/test_ambari.py
cilium/tests/common.py
...e/datadog_checks/base/checks/openmetrics/mixins.py
ibm_db2/tests/test_integration_e2e.py
dotnetclr/datadog_checks/dotnetclr/dotnetclr.py
datadog_checks_dev/datadog_checks/dev/env.py
druid/tests/test_integration_e2e.py
couch/tests/test_couchv2.py
cilium/datadog_checks/cilium/__about__.py
... and 794 more

@AlexandreYang AlexandreYang changed the title Move stubs to dev Move stubs to dev (Part 1/2) Mar 27, 2020
This was referenced Mar 27, 2020
Closed
Closed
@AlexandreYang AlexandreYang marked this pull request as ready for review March 27, 2020 17:11
@AlexandreYang AlexandreYang requested review from a team as code owners March 27, 2020 17:11
@hithwen
Copy link
Contributor

hithwen commented Mar 30, 2020

Many tests are broken, too many to be due to flakeyness I think

@AlexandreYang
Copy link
Member Author

AlexandreYang commented Mar 30, 2020 •
edited
Loading

Many tests are broken, too many to be due to flakeyness I think

@hithwen You mean for this build ? I created this separate CI build to get the results since the PR is in 2 parts: #6176 ?

Only envoy fails, but it's for another reason since it's breaking on master too.

florimondmanca
florimondmanca previously approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Checked out the code locally too, everything looks fine.

Can you confirm this the plan?

  1. Merge Move stubs to dev (Part 2/2) #6175 into this PR
  2. Merge this PR into master

Should we plan for a follow-up PR to upgrade the base imports in integrations tests to dev?

@ofek ofek changed the title Move stubs to dev (Part 1/2) Move Agent stubs to ddev (Part 1/2) Mar 30, 2020
ofek
ofek requested changes Mar 30, 2020
View reviewed changes
datadog_checks_dev/datadog_checks/dev/stubs/aggregator.py
Comment on lines 10 to +12
from datadog_checks.base.stubs.common import HistogramBucketStub, MetricStub, ServiceCheckStub
from datadog_checks.base.stubs.similar import build_similar_elements_msg

from ..utils.common import ensure_unicode, to_native_string
from datadog_checks.base.utils.common import ensure_unicode, to_native_string
Copy link
Contributor

Choose a reason for hiding this comment

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

from .stubs ... no need to import from base no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually agreed with @ofek, looks like there are lots of opportunities for updating imports from base to dev in the moved files.

datadog_checks_dev/datadog_checks/dev/stubs/log.py
@@ -3,7 +3,7 @@
# Licensed under a 3-clause BSD style license (see LICENSE)
import logging

from ..log import CheckLoggingAdapter as AgentLoggingAdapter
from datadog_checks.base.log import CheckLoggingAdapter as AgentLoggingAdapter
Copy link
Contributor

Choose a reason for hiding this comment

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

We should refactor to never need to import from base

@stale
Copy link

stale bot commented Apr 30, 2020

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

@stale stale bot added the stale label Apr 30, 2020
@dd-devflow dd-devflow bot deleted the alex/move_stubs branch February 7, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florimondmanca florimondmanca florimondmanca left review comments

@ofek ofek ofek requested changes

@hithwen hithwen hithwen approved these changes

Assignees
No one assigned
Labels
dev/tooling integration/datadog_checks_base integration/datadog_checks_dev stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexandreYang @hithwen @ofek @florimondmanca