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

Relax validation for smseagle hostname #10141

Merged
merged 2 commits into from Apr 27, 2019

Conversation

Projects
None yet
2 participants
@petracvv
Copy link
Contributor

commented Apr 24, 2019

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

When trying to setup the smseagle transport, the validator was checking for a vaild URL in the form validation. However, the transport expects a hostname not a URL because it constructs the URL itself:

$url = 'http://' . $opts['url'] . '/index.php/http_api/send_sms?' . http_build_query($params);

This pull request relaxes the validation to accept a string so IP addresses and Hostnames are accepted by the form.

@petracvv

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

It might be better to rename the attribute, but I am not sure what that would entail.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

Hi @petracvv
Renaming the attribute would kill the alerts for any user that is running smseagle now (and probably configured it before the stricter checks on the url ...).
So I would suggest to avoid renaming it for now.

You should still add these to the PR:

  • Change the description of the field to make it clear for user that it is a host.
  • Check the documentation and adapt it if necessary.
    Thanx
@petracvv

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

OK. I updated the property description and the docs. Let me know if I should look at anything else.

@PipoCanaja
Copy link
Contributor

left a comment

LGTM

@PipoCanaja PipoCanaja merged commit 2df2956 into librenms:master Apr 27, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

Improved option naming & validation for smseagle (librenms#10141)
* Relax validation for smseagle hostname
* Update property description and documentation

spencerbutler added a commit to spencerbutler/librenms that referenced this pull request May 21, 2019

Improved option naming & validation for smseagle (librenms#10141)
* Relax validation for smseagle hostname
* Update property description and documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.