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

Data Collector server URL validation, and disable on host down #5076

Merged
merged 2 commits into from
Jul 5, 2016

Conversation

adamleff
Copy link
Contributor

@adamleff adamleff commented Jul 1, 2016

If a user configured data_collector.server_url in their Chef config
as an empty string, we blissfully passed it along to Chef::HTTP
which would eventually raise a TypeError when trying to dup the
URI's host. This change validates the server_url when setting up
the Data Collector and gives helpful error messages to the user.

We also count Errno::EHOSTDOWN as an error worthy of disabling
the Data Collector reporter for a given run if the user so chooses.

If a user configured data_collector.server_url in their Chef config
as an empty string, we blissfully passed it along to Chef::HTTP
which would eventually raise a TypeError when trying to dup the
URI's host. This change validates the server_url when setting up
the Data Collector and gives helpful error messages to the user.

We also count Errno::EHOSTDOWN as an error worthy of disabling
the Data Collector reporter for a given run if the user so chooses.
@adamleff adamleff force-pushed the adamleff/ipo-277-uri-validation branch from 195ac87 to 1e70e79 Compare July 3, 2016 17:26

def validate_data_collector_server_url!
raise Chef::Exceptions::ConfigurationError,
"data_collector.server_url is configured but empty. Please supply a valid URL." if data_collector_server_url.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

"is configured but empty" sounds a little technical, can we just say "is empty." ?

@mwrock
Copy link
Member

mwrock commented Jul 5, 2016

👍

@btm
Copy link
Contributor

btm commented Jul 5, 2016

👍 with nits

@thommay
Copy link
Contributor

thommay commented Jul 5, 2016

@btm got all the nits ; 👍 once they're done

@adamleff
Copy link
Contributor Author

adamleff commented Jul 5, 2016

appveyor error appears unrelated, internal discussion agrees we can merge, so merge I shall...

@adamleff adamleff merged commit 0d584ca into master Jul 5, 2016
@adamleff adamleff deleted the adamleff/ipo-277-uri-validation branch July 5, 2016 19:29
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 25, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants