Skip to content

Conversation

@andrii-suse
Copy link
Collaborator

No description provided.

@andrii-suse andrii-suse changed the title Add environment variable OSC_HTTP_MANUAL_REVIEW Add environment variable OSC_HTTP_MANUAL_APPROVE Nov 28, 2025
http.client.print(40 * '-')
http.client.print(method, url)

if method != "GET" and os.environ['OSC_HTTP_MANUAL_APPROVE']:
Copy link
Contributor

Choose a reason for hiding this comment

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

os.environ['OSC_HTTP_MANUAL_APPROVE'] may raise KeyError.

http.client.print(method, url)

if method != "GET" and os.environ['OSC_HTTP_MANUAL_APPROVE']:
print("osc is going to send "+ method +" request to " + urlopen_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use a f-string instead?
Maybe drop this print and rely on the prompt below?

if method != "GET" and os.environ['OSC_HTTP_MANUAL_APPROVE']:
print("osc is going to send "+ method +" request to " + urlopen_url)
if data:
print(data.decode("utf-8"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This may fail. It is not guaranteed that the data is utf-8 text.

Comment on lines +349 to +352
y = input("press 'y' to continue or any other key to cancel")
if y != 'y' and y != 'Y':
raise Exception("Manual abort")

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use functions and exceptions osc already offers:
(please excuse the wrong indendation, I've copied the code from a different place and slightly modified it to fit the scenario)

               from .output import get_user_input

               reply = get_user_input(
                    f"Do you want to  make '{method}' request to '{urlopen_url}'?",
                    answers={"y": "yes", "n": "no"},
                    default_answer="n",
                )
                if reply == "n":
                    raise oscerr.UserAbort()

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