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

Properly use skip_proxy for instance configuration #1880

Merged
merged 4 commits into from
Jul 17, 2018
Merged

Properly use skip_proxy for instance configuration #1880

merged 4 commits into from
Jul 17, 2018

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Jul 12, 2018

@ofek ofek requested a review from a team as a code owner July 12, 2018 21:53
@masci
Copy link
Contributor

masci commented Jul 12, 2018 •
edited
Loading

Let's add tests for get_instance_proxy before merging

derekwbrown
derekwbrown requested changes Jul 12, 2018
View reviewed changes
Copy link
Contributor

@derekwbrown derekwbrown left a comment

Choose a reason for hiding this comment

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

Also needs change to http_proxy (at least), which also reads the config. If we're not going to use that, it should be removed (so it's not confusing). And, update the example yamls

datadog_checks_base/datadog_checks/checks/base.py
)

if deprecated_skip is not None:
self._log_deprecation('no_proxy')
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure that no_proxy is the one that's intended to be deprecated? My read was that skip_proxy was the one to be deprecated.

Either way, since the ENV variable is NO_PROXY, IMO the valid one should be no_proxy and the deprecated skip_proxy. They should be the same or it will be more confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually began the deprecation process for no_proxy a little while back because that and the environment variable NO_PROXY that the agent also respects mean different things. The env var is a comma separated list of URLs to ignore proxy settings for, but our configuration flag actually semantically meant skip all proxy settings.

gzussa
gzussa approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@gzussa gzussa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding tests and a description to the PR. It makes the reviews way easier 💯.

@nmuesch nmuesch changed the title mirror agent proxy behavior Properly use skip_proxy for instance configuration Jul 17, 2018
@nmuesch nmuesch merged commit fc10be6 into master Jul 17, 2018
@nmuesch nmuesch added this to the 6.3.4 milestone Jul 17, 2018
@ofek ofek deleted the ofek/proxy-fix branch July 17, 2018 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekwbrown derekwbrown derekwbrown requested changes

@gzussa gzussa gzussa approved these changes

Assignees
No one assigned
Labels
integration/datadog_checks_base
Projects
None yet
Milestone
6.3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@ofek @masci @gzussa @derekwbrown @nmuesch