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

Fix: Validate headless and host options #2627

Closed
wants to merge 7 commits into from

Conversation

ilyabe
Copy link

@ilyabe ilyabe commented Mar 12, 2024

Fixes #2575

Based on the discussion in #2610, validates the --headless and --host options.

  • If --headless is passed, then --host is required
  • If --host value doesn't include protocol, an error message is printed
  • Updated all tests that had --headless without --host
  • Added tests for the new error messages

These changes are only for the CLI. I'll make a separate PR for the webui.

If there's a better approach or anything to improve, I'm happy to.

@cyberw
Copy link
Collaborator

cyberw commented Mar 15, 2024

If --headless is passed, then --host is required

--host should not be required, because it can also be set on the User class. Lets just validate it if it is set,

Another problem is, what if it is not an http test at all? Maybe this solution is more problematic than I thought...

@ilyabe
Copy link
Author

ilyabe commented Mar 19, 2024

what if it is not an http test at all?

Then this is trickier. We could have a list of supported protocols (e.g. http, https, ftp, ssh, irc, etc.) but that might be too limiting? We could check that :// is in the host but I'm not sure all protocols specify a :// syntax. Maybe things are fine as is?

@ilyabe
Copy link
Author

ilyabe commented Mar 19, 2024

I updated the PR with one more approach. It uses urllib's urlparse to check that the parsed host has a scheme value. Just another option. If there are any issues with this, let me know :)

@cyberw
Copy link
Collaborator

cyberw commented Mar 21, 2024

what if it is not an http test at all?

Then this is trickier. We could have a list of supported protocols (e.g. http, https, ftp, ssh, irc, etc.) but that might be too limiting? We could check that :// is in the host but I'm not sure all protocols specify a :// syntax. Maybe things are fine as is?

I wouldnt necessarily call it "fine" :) but the current design makes it really hard to fix :-/

I wonder... Perhaps HttpUser could validate its host attribute on initialization and stop the test if it is not a valid prefix? (probably we should allow None, but that is not the key issue) Or maybe throwing an exception is enough to stop the user from being spawned. I dont really have time to explore how feasible that would be, but that way different Users can use different schemes.

@cyberw cyberw added the stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it label May 2, 2024
Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Locust spams error if Host is invalid
2 participants