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 warning to [p]set api if <>s are included in secret #6265

Open
wants to merge 5 commits into
base: V3/develop
Choose a base branch
from

Conversation

giplgwm
Copy link
Contributor

@giplgwm giplgwm commented Nov 1, 2023

Description of the changes

Closes #6253
For every given secret, check if its wrapped in <>, if it is, add a message to an embed with a warning that that secret may not be set properly. I tested this on a local instance of the bot and it seems to be working as expected. The warning is also logged

Have the changes in this PR been tested?

Yes

@github-actions github-actions bot added the Category: Core - Bot Commands This is related to core commands (Core and CogManagerUI cog classes). label Nov 1, 2023
@Flame442 Flame442 added the hacktoberfest-accepted Used to mark a PR as valid Hacktoberfest contribution. DO NOT REMOVE UNTIL END OF NOVEMBER (sic!)! label Nov 1, 2023
redbot/core/core_commands.py Outdated Show resolved Hide resolved
redbot/core/core_commands.py Show resolved Hide resolved
redbot/core/core_commands.py Outdated Show resolved Hide resolved
redbot/core/core_commands.py Outdated Show resolved Hide resolved
redbot/core/core_commands.py Outdated Show resolved Hide resolved
@giplgwm giplgwm requested a review from Drapersniper November 4, 2023 23:37
@giplgwm
Copy link
Contributor Author

giplgwm commented Nov 16, 2023

@Drapersniper I added the changes requested in your comments as another commit on the PR, thanks

@Kreusada Kreusada self-assigned this Feb 13, 2024
@Kreusada Kreusada added the Type: Enhancement Something meant to enhance existing Red features. label Feb 13, 2024
Copy link
Member

@Kreusada Kreusada left a comment

Choose a reason for hiding this comment

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

Just a couple of suggested changes to improve readability.

Comment on lines +3640 to +3643
"You may have failed to properly format your {api_service_name}. If you were told to enter a key"
" with an example such as [p]set api {service} api_key <your_api_key_here>, and your API key"
" was HREDFGWE, make sure to run [p]set api {service} api_key HREDFGWE, and not "
" [p]set api {service} api_key <HREDFGWE>"
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping the command examples with inline marks may be appropriate to improve readability.

Suggested change
"You may have failed to properly format your {api_service_name}. If you were told to enter a key"
" with an example such as [p]set api {service} api_key <your_api_key_here>, and your API key"
" was HREDFGWE, make sure to run [p]set api {service} api_key HREDFGWE, and not "
" [p]set api {service} api_key <HREDFGWE>"
"You may have failed to properly format your {api_service_name}. If you were told to enter a key"
" with an example such as `[p]set api {service} api_key <your_api_key_here>`, and your API key"
" was `HREDFGWE`, make sure to run `[p]set api {service} api_key HREDFGWE` and not "
" `[p]set api {service} api_key <HREDFGWE>`."

await ctx.send(_("`{service}` API tokens have been set.").format(service=service))
await ctx.send(
_("`{service}` API tokens have been set.").format(service=service),
embed=embed if len(embed.fields) > 0 else None,
Copy link
Member

Choose a reason for hiding this comment

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

It isn't necessary to give this warning inside of an embed, when there is already text content (it would make things more complicated, such as having to check if an embed is requested in the current channel). Instead, you could add line breaks under the original message and put the warning text there. Something like this would suffice:

angle_bracket_warning = None # define this before iterating through tokens
for api_service_name, token in tokens.items():
    ...
message = _("`{service}` API tokens have been set.").format(service=service)
if angle_bracket_warning:
    message += "\n\n" + _("**Warning:** ") + _(angle_bracket_warning)
await ctx.send(message)

Remember to remove the embed definition on line 3636.

@Kreusada Kreusada added the QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. label Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core - Bot Commands This is related to core commands (Core and CogManagerUI cog classes). hacktoberfest-accepted Used to mark a PR as valid Hacktoberfest contribution. DO NOT REMOVE UNTIL END OF NOVEMBER (sic!)! QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. Type: Enhancement Something meant to enhance existing Red features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning to [p]set api if <>s are included in secret
4 participants