Skip to content

Remove interactive redirect ask#7861

Merged
bmw merged 7 commits intomasterfrom
no-redirect-ask
Mar 31, 2020
Merged

Remove interactive redirect ask#7861
bmw merged 7 commits intomasterfrom
no-redirect-ask

Conversation

@ohemorange
Copy link
Copy Markdown
Contributor

Fixes #7594.

Removes the code asking interactively if the user would like to add a redirect.

if enhancement_name in supported:
if ask_redirect:
if config_name == "redirect" and config_value is None:
config_value = enhancements.ask(enhancement_name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change also has the effect of no longer setting up a redirect unless the user manually requested it with --redirect.

To fix this, I'd recommend keeping code like this here:

if config_name == "redirect" and config_value is None:
    config_value = True

@bmw
Copy link
Copy Markdown
Member

bmw commented Mar 25, 2020

Oh! I think we should also update the help text at

help="Automatically redirect all HTTP traffic to HTTPS for the newly "
"authenticated vhost. (default: Ask)")
helpful.add(
"security", "--no-redirect", action="store_false", dest="redirect",
default=flag_default("redirect"),
help="Do not automatically redirect all HTTP traffic to HTTPS for the newly "
"authenticated vhost. (default: Ask)")
.

EDIT: We should update --no-redirect too.

default=flag_default("redirect"),
help="Do not automatically redirect all HTTP traffic to HTTPS for the newly "
"authenticated vhost. (default: Ask)")
"authenticated vhost. (default: False for enhance, True for renew and run)")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I decided not to remove these from security because they show up next to each other during all so I wanted to find a way that makes sense even if they're next to each other. I considered adding this one under redirect and run but honestly think we have enough flags there and we didn't need it there before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works for me!

In addition to my same comment about renew vs. install, my only concern here is what does true and false mean here in the context of a --no-redirect flag? Other --no-* flags we have such as --no-verify-ssl's help says:

  --no-verify-ssl       Disable verification of the ACME server's certificate.
                        (default: False)

where False is meant to mean the flag is not applied so the server's cert is verified. We have the opposite meaning with --no-redirect here.

To clear this up, what do you think about being very explicit and saying something like "enable a redirect with install and run, but not with enhance"? If we make this change I think we should probably do the same thing with the --redirect help.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I love that, stateful booleans are inherently more confusing than idempotent statements.

Copy link
Copy Markdown
Member

@bmw bmw left a comment

Choose a reason for hiding this comment

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

Two nits about the help text, but this otherwise LGTM!

default=flag_default("redirect"),
help="Automatically redirect all HTTP traffic to HTTPS for the newly "
"authenticated vhost. (default: Ask)")
"authenticated vhost. (default: False for enhance, True for renew and run)")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this flag will have any effect with certbot renew.

Suggested change
"authenticated vhost. (default: False for enhance, True for renew and run)")
"authenticated vhost. (default: False for enhance, True for install and run)")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah yes I did mean install here, thanks!

default=flag_default("redirect"),
help="Do not automatically redirect all HTTP traffic to HTTPS for the newly "
"authenticated vhost. (default: Ask)")
"authenticated vhost. (default: False for enhance, True for renew and run)")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works for me!

In addition to my same comment about renew vs. install, my only concern here is what does true and false mean here in the context of a --no-redirect flag? Other --no-* flags we have such as --no-verify-ssl's help says:

  --no-verify-ssl       Disable verification of the ACME server's certificate.
                        (default: False)

where False is meant to mean the flag is not applied so the server's cert is verified. We have the opposite meaning with --no-redirect here.

To clear this up, what do you think about being very explicit and saying something like "enable a redirect with install and run, but not with enhance"? If we make this change I think we should probably do the same thing with the --redirect help.

Copy link
Copy Markdown
Member

@bmw bmw left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm glad to have yet another issue we've been talking about for a long time resolved.

@bmw bmw merged commit dbda499 into master Mar 31, 2020
@bmw bmw deleted the no-redirect-ask branch March 31, 2020 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove interactive redirect ask

2 participants