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

add is_secret option for custom args to be shown in the web UI masked #2284

Merged

Conversation

mzhukovs
Copy link
Contributor

@mzhukovs mzhukovs commented Jan 7, 2023

Thought it would be nice, especially for live demo purposes, to be able to quickly indicate when adding custom arguments that are to be exposed in the web UI which should be password masked text inputs, e.g. when you want to use the Web UI to run the load tests under different user logins, so the user password text input field should be a password input.

<label for="{{key}}">{{key}}</label>
<input type="text" name="{{key}}" id="{{key}}" class="val" value="{{value}}" /><br>
<input type="{{'password' if value[1] else 'text'}}" name="{{key}}" id="{{key}}" class="val" value="{{value[0]}}" /><br>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a little too weird/un-obvious. Maybe use a dict instead of a tuple?

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 agree @cyberw , it even felt wrong as I was typing it. How about a NamedTuple instead? I also threw in help_text there to display in same style as the other default inputs displayed in the UI if provided. Please refer to new commit 64e9303

Thank you for your consideration

@cyberw
Copy link
Collaborator

cyberw commented Jan 7, 2023

Sounds useful. I’m thinking maybe the option should just beis_password instead of is_secret?

@mzhukovs
Copy link
Contributor Author

mzhukovs commented Jan 8, 2023

Sounds useful. I’m thinking maybe the option should just beis_password instead of is_secret?

My thought was "secret" is a lot more general than "password" - e.g. what if the user wanted to have an API Key as a masked input? It's not really a password, but it is a secret - so feels a little more intuitive for use with parser.add_argument() but of course I defer to you.

@mzhukovs
Copy link
Contributor Author

mzhukovs commented Jan 8, 2023

Also, is it just me or does the CSS for the footer need to be updated to make it sticky (without overlapping with actual content)?

@cyberw
Copy link
Collaborator

cyberw commented Jan 8, 2023

Sounds useful. I’m thinking maybe the option should just beis_password instead of is_secret?

My thought was "secret" is a lot more general than "password" - e.g. what if the user wanted to have an API Key as a masked input? It's not really a password, but it is a secret - so feels a little more intuitive for use with parser.add_argument() but of course I defer to you.

because i’m used to html is_password made more sense to me :) but it doesnt really matter.

@cyberw
Copy link
Collaborator

cyberw commented Jan 8, 2023

@heyman what do you think?

@cyberw
Copy link
Collaborator

cyberw commented Jan 8, 2023

Also, is it just me or does the CSS for the footer need to be updated to make it sticky (without overlapping with actual content)?

Idk :)

@mzhukovs
Copy link
Contributor Author

mzhukovs commented Jan 9, 2023

Also, is it just me or does the CSS for the footer need to be updated to make it sticky (without overlapping with actual content)?

Idk :)

Yes definitely, please refer to latest commit 52fe074 to fix this - also I noticed the "About" popup (from the link in the footer) was getting covered up by the header so adjusted the z-index to prevent that as well

@heyman
Copy link
Member

heyman commented Jan 9, 2023

Sounds reasonable! I'd prefer is_secret over is_password I think.

We should also add something about this under the Custom arguments section in the documentation for extending Locust (which I also think we should rename to "Extending Locust" instead of "Event hooks" btw, see my comment at: 9469486#r95553244)

@cyberw
Copy link
Collaborator

cyberw commented Jan 11, 2023

I can merge this now if there is nothing more to add.

@mzhukovs
Copy link
Contributor Author

Sounds reasonable! I'd prefer is_secret over is_password I think.

We should also add something about this under the Custom arguments section in the documentation for extending Locust (which I also think we should rename to "Extending Locust" instead of "Event hooks" btw, see my comment at: 9469486#r95553244)

Yep this was already done, please refer to this part of the first commit: e71eb8f#diff-0a5d25e3b717d98c55f177095b87d64583ccfb2e076c91c03e4afcbf155864dd

Which the docs file docs/extending-locust.rst references.

@heyman
Copy link
Member

heyman commented Jan 11, 2023

Yep this was already done, please refer to this part of the first commit: e71eb8f#diff-0a5d25e3b717d98c55f177095b87d64583ccfb2e076c91c03e4afcbf155864dd

Which the docs file docs/extending-locust.rst references.

Ah, yes, I forgot that the example file is inlined into the documentation 👍

@cyberw cyberw merged commit 9300e7f into locustio:master Jan 11, 2023
@mzhukovs mzhukovs deleted the add-option-for-password-input-in-web-ui branch January 11, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants