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

Return an error if the client name is not filled #19

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Return an error if the client name is not filled #19

merged 1 commit into from
Dec 5, 2016

Conversation

AlexisMontagne
Copy link
Member

Fix #18

mihaitodor
mihaitodor reviewed Dec 3, 2016
View reviewed changes
Copy link
Contributor

@mihaitodor mihaitodor left a comment

Choose a reason for hiding this comment

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

The changes look OK, but I think we can still run into this issue if SENSU_CLIENT_NAME is set to "" (or not set?)

@AlexisMontagne
Copy link
Member Author

AlexisMontagne commented Dec 5, 2016 •
edited
Loading

@mihaitodor, if you use one of the constructor (NewConfigFromFile or NewConfigFromFlagSet), an error will be returned if the computed name is the empty string.

@AlexisMontagne AlexisMontagne merged commit c21dd9a into master Dec 5, 2016
@AlexisMontagne AlexisMontagne deleted the am-error-client-name branch December 5, 2016 17:18
@mihaitodor
Copy link
Contributor

@AlexisMontagne yep, I was thinking about people who would create Config instances manually, without using NewConfigFromFile or NewConfigFromFlagSet (for whatever reason), but yeah, if they don't use the provided API, then it's their problem :) OTOH, the Sensu Server should probably do a bit more validation.

Please go ahead and merge this PR if you wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaitodor mihaitodor mihaitodor left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlexisMontagne @mihaitodor