Skip to content

[network] Refactor: split check_linux methods into different class#13109

Merged
hithwen merged 7 commits into
masterfrom
js/split-check-linux
Oct 13, 2022
Merged

[network] Refactor: split check_linux methods into different class#13109
hithwen merged 7 commits into
masterfrom
js/split-check-linux

Conversation

@hithwen

@hithwen hithwen commented Oct 12, 2022

Copy link
Copy Markdown
Contributor

split of #13102 only extracting the linux check for easier review

Extract check linux into a different class
Rename methods that will remain in the parent class
Noticed that some parts of the linux check have no coverage, working on it on a separate PR #13117

@hithwen hithwen changed the title [network] Refactor: split check_linux methods into different classes [network] Refactor: split check_linux methods into different class Oct 12, 2022
@hithwen
hithwen marked this pull request as ready for review October 13, 2022 11:27
@hithwen
hithwen requested review from a team as code owners October 13, 2022 11:27
@DataDog DataDog deleted a comment from codecov Bot Oct 13, 2022
@DataDog DataDog deleted a comment from codecov Bot Oct 13, 2022
@codecov

codecov Bot commented Oct 13, 2022

Copy link
Copy Markdown

Codecov Report

Merging #13109 (b0665e7) into master (6dd55c7) will increase coverage by 0.07%.
The diff coverage is 84.26%.

Flag Coverage Δ
network 85.78% <84.26%> (+1.74%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.


def check(self, _):
"""
_check_linux can be run inside a container and still collects the network metrics from the host

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Outdated docstring (this is no longer _check_linux).

Comment on lines +22 to +25
try:
import fcntl
except ImportError:
fcntl = None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks unused.

Comment on lines +456 to +457
@mock.patch('datadog_checks.network.network.Platform.is_bsd', return_value=False)
@mock.patch('datadog_checks.network.network.Platform.is_linux', return_value=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be nice if we could refactor this into a pytest fixture / decorator.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a rabbithole I don't want to get into right now

@alopezz alopezz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me

@hithwen
hithwen merged commit d550215 into master Oct 13, 2022
@hithwen
hithwen deleted the js/split-check-linux branch October 13, 2022 14:00
steveny91 pushed a commit that referenced this pull request Oct 27, 2022
@hithwen

hithwen commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

This PR introduces a bug in solaris that has been fixed on #13319

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