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

Handle empty value for config set proxy #2984

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Handle empty value for config set proxy #2984

merged 2 commits into from
Mar 21, 2024

Conversation

dheyay
Copy link
Contributor

@dheyay dheyay commented Mar 8, 2024 •
edited
Loading

Why is this needed?

Currently the user can set config proxy value to empty using pro config set http_proxy=
This fixes that, since the given proxy is invalid, an error is shown.

Fixes: [#2925]

Test Steps

Checklist

  • I have updated or added any unit tests accordingly
  • I have updated or added any integration tests accordingly
  • Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

  • Yes
  • No

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024 •
edited by dheyay
Loading

🌎 This PR changes translatable messages. 🌏

Please select which scenarios apply. For further explanation, please read our policy on message changes.

  • New messages are being added.
    • We will ask translators to take a look and add translations if they have time, but it will not block this PR.
  • Existing messages are being changed.
    • ⚠️ Please add a comment with justification of why messages are being altered.
    • If the changes are trivial (e.g. a typo fix), then translations must be preserved.
    • If the changes are substantial, then we will ask translators to take a look and update translations if they have time, but it will not block this PR.
  • Existing messages are being deleted.
    • No special action needed.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024 •
edited
Loading

Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference)

GitHub Issues:

Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references)

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

@dheyay
Copy link
Contributor Author

dheyay commented Mar 12, 2024 •
edited
Loading

Might need updated translations since a new error message was added when config set value is not given.

Open to suggestions for a solution to re-use other error messages than creating a new one.

@dheyay dheyay marked this pull request as ready for review March 12, 2024 18:33
@dheyay dheyay changed the base branch from next-v32 to main March 15, 2024 16:35
orndorffgrant
orndorffgrant reviewed Mar 15, 2024
View reviewed changes
Copy link
Collaborator

@orndorffgrant orndorffgrant left a comment

Choose a reason for hiding this comment

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

Thanks!
Let's name the error and message more specifically.
Otherwise, looks good!

uaclient/messages/__init__.py Outdated Show resolved Hide resolved
uaclient/exceptions.py Outdated Show resolved Hide resolved
uaclient/cli/__init__.py Outdated Show resolved Hide resolved
@dheyay dheyay force-pushed the http-proxy-set branch 2 times, most recently from 42b3f5d to 0720a13 Compare March 15, 2024 20:28
@dheyay
Copy link
Contributor Author

dheyay commented Mar 15, 2024

Updated @orndorffgrant

@renanrodrigo
Copy link
Member

I am sad about this nit, but:
could you just adapt the commit message comment to say
Fixes: #2925
instead of
Fixes Bug: [#2925]

For CI reasons, we always use that format :/

lucasmoura
lucasmoura reviewed Mar 20, 2024
View reviewed changes
uaclient/cli/__init__.py
@@ -690,6 +690,9 @@ def action_config_set(args, *, cfg, **kwargs):
raise exceptions.InvalidArgChoice(
arg="<key>", choices=", ".join(config.UA_CONFIGURABLE_KEYS)
)
if not set_value.strip():
subparser.print_help()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to print the help message here ? Maybe just stating that the user provided an invalid empty value is enough.

What do you think @renanrodrigo ?

Copy link
Member

Choose a reason for hiding this comment

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

This is following what we do for all other errors on setting config values. I think this looks good as is.

renanrodrigo
renanrodrigo approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@renanrodrigo renanrodrigo left a comment

Choose a reason for hiding this comment

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

LGTM

@renanrodrigo renanrodrigo merged commit 6e9dcdc into main Mar 21, 2024
21 of 24 checks passed
@renanrodrigo renanrodrigo deleted the http-proxy-set branch March 21, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orndorffgrant orndorffgrant orndorffgrant left review comments

@lucasmoura lucasmoura lucasmoura left review comments

@renanrodrigo renanrodrigo renanrodrigo approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@dheyay @renanrodrigo @lucasmoura @orndorffgrant