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 pip instructions #698

Merged
merged 23 commits into from Mar 3, 2021
Merged

Add pip instructions #698

merged 23 commits into from Mar 3, 2021

Conversation

ohemorange
Copy link
Contributor

@ohemorange ohemorange commented Feb 25, 2021

Questions:

  • Should we give people the command to upgrade certbot monthly via cron and remove [Optional]?
  • Should we switch to recommending people blow away the venv each time?
  • Should the upgrade instruction be down below with renewal, so people can do the most important things first? It's a little more work (it'll need to be moved into a different partial) but super doable.
  • We don't mention third-party plugins elsewhere; should we remove that from here?
  • What should the system name in the uri and the dropdown/title be?

dropdown and heading:
Screenshot from 2021-02-25 15-19-36

default instructions:
Screenshot from 2021-02-25 15-17-18
Screenshot from 2021-02-25 15-17-34
Screenshot from 2021-02-25 15-17-44

wildcard instructions:
Screenshot from 2021-02-25 15-18-50

@ohemorange ohemorange marked this pull request as ready for review February 25, 2021 22:36
@ohemorange ohemorange requested a review from bmw February 25, 2021 23:16
Copy link
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.

Questions:

For all of these I went ahead and shared my opinion, however, I'm also curious about what you think as I think you have a good instinct for this kind of stuff.

Should we give people the command to upgrade certbot monthly via cron and remove [Optional]?

I personally don't think so. I think the chance of things breaking when upgrading is too high. For example, cryptography could require a newer version of rust or dropped support for the user's version of OpenSSL.

Should we switch to recommending people blow away the venv each time?

I wrote more about this on the Google doc, but I personally think we should either just recommend pip install --upgrade or blowing away the venv. If we feel the need to document both, then I think pip install --upgrade maybe isn't reliable enough and we should just recreate the venv.

Should the upgrade instruction be down below with renewal, so people can do the most important things first? It's a little more work (it'll need to be moved into a different partial) but super doable.

I think that'd be nice!

We don't mention third-party plugins elsewhere; should we remove that from here?

I think so. I think on the default tab we shouldn't have this step but on the wildcard tab we'd have the instructions to install the DNS plugins.

What should the system name in the uri and the dropdown/title be?

Maybe make the system name in the uri pip and the dropdown/title "Other Linux (from source)" while changing the other "Other Linux" option to "Other Linux (snapd)"?

_scripts/instruction-widget/install.js Outdated Show resolved Hide resolved
_scripts/instruction-widget/templates/install/pip.html Outdated Show resolved Hide resolved
_scripts/instruction-widget/templates/install/pip.html Outdated Show resolved Hide resolved
_scripts/instruction-widget/templates/install/pip.html Outdated Show resolved Hide resolved
_scripts/instruction-widget/templates/install/pip.html Outdated Show resolved Hide resolved
_scripts/instruction-widget/templates/install/pip.html Outdated Show resolved Hide resolved
@ohemorange
Copy link
Contributor Author

ohemorange commented Mar 1, 2021

Should we give people the command to upgrade certbot monthly via cron and remove [Optional]?

I personally don't think so. I think the chance of things breaking when upgrading is too high. For example, cryptography could require a newer version of rust or dropped support for the user's version of OpenSSL.

  • Ack, that's a good point. Since we're only updating some of the packages and can't be sure people have updated their system packages.

Should we switch to recommending people blow away the venv each time?

I wrote more about this on the Google doc, but I personally think we should either just recommend pip install --upgrade or blowing away the venv. If we feel the need to document both, then I think pip install --upgrade maybe isn't reliable enough and we should just recreate the venv.

Hm, looking at your comments on the Google doc. Did certbot-auto actually blow away the venv every time? I thought we had eventually fixed it somehow so it didn't have to. I know we often used to run into issues while testing, but that's probably because we were only updating certbot's source directly by pulling from master, which certainly wouldn't pull in any necessary updates to dependencies.

EDIT 3: wow, we really did just blow away the old venv every time. I still have a little hesitation from a UX perspective, since it'll be a little longer, and now the user is probably actively waiting for it to finish instead of having it run from cron. But I'd much rather mimic existing workflows that we know haven't been actively problematic, rather than having to deal with new unknown issues. So let's blow it away! Whoo!

Edit 4: Ah...just noticed a few things. Easiest is that that rm command needs sudo. Next is that the current instruction doesn't mention the dns packages at all, so if we left it we'd have to fix that. But that leads into finally: the existing installation steps are scattered among various steps: "Set up a Python virtual environment", "Install Certbot", and possibly "Install correct DNS plugin". (Which, further thing, here and possibly also in snapd, "Prepare the Certbot command" should probably be after "Install correct DNS plugin".) What are the chances that people just shoved the DNS credentials into the venv, which we've now told them to delete? We could leave the wording as is, saying "repeat all installation instructions." If we keep the --upgrade, it's not so bad to conditionally put in a certbot-dns-x entry if we're on dns instructions. We could also tell people explicitly which steps to repeat; I don't think we should try to take all those contents and shove them into this step. Leaning towards this last one, though it's a little messy as instructions go. So weighing that against the potential pitfalls of just telling people to run pip --upgrade certbot[s]. Ooh we could even extract the things with certbot in the name from pip freeze and pass those back in, instead of having to conditionally include certbot-dns-x, making the command copy-pastable.

Should the upgrade instruction be down below with renewal, so people can do the most important things first? It's a little more work (it'll need to be moved into a different partial) but super doable.

I think that'd be nice!

  • Cool, I will do this.

We don't mention third-party plugins elsewhere; should we remove that from here?

I think so. I think on the default tab we shouldn't have this step but on the wildcard tab we'd have the instructions to install the DNS plugins.

  • Great, that's where I was leaning as well.

What should the system name in the uri and the dropdown/title be?

Maybe make the system name in the uri pip and the dropdown/title "Other Linux (from source)" while changing the other "Other Linux" option to "Other Linux (snapd)"?

  • Hm, I think since we've already been using "pip" for Other Linux's uri for some reason, using pip for this might break urls if anyone's bookmarked or posted it somewhere. I like the idea of using other linux () for both of these. I guess since it's python installing from pip is technically from source, but that feels a little weird. Other Linux (pip)? Other Linux (venv)? I think (pip) is pretty analogous to (snapd). Maybe pip-venv for the uri?

EDIT: actually it turns out "Other Linux" is just our current fallthrough; it's just going to whatever we want the default to be. In light of that, I think I'd like to have a specific pip option much in the same we have a specific snapd option, so people can scroll quickly through to find the pip instructions if they're specifically looking for the pip instructions. And then we can still have two Other Linuxes if we want, do we even need a fallthrough? Why do we even have a fallthrough, it's a dropdown, let's just specifically say where those two other linuxes go.

EDIT 2: Or were you thinking get rid of the plain snapd option and just have Other Linux (pip) and Other Linux (snapd)? I wouldn't want to erroneously indicate that those instructions won't work on systems that are listed in the menu.

@ohemorange ohemorange changed the title Add pip instructions (barring DNS instructions fixes) Add pip instructions Mar 1, 2021
@kallisti5

This comment was marked as duplicate.

Copy link
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.

Hm, looking at your comments on the Google doc. Did certbot-auto actually blow away the venv every time? I thought we had eventually fixed it somehow so it didn't have to. I know we often used to run into issues while testing, but that's probably because we were only updating certbot's source directly by pulling from master, which certainly wouldn't pull in any necessary updates to dependencies.

EDIT 3: wow, we really did just blow away the old venv every time. I still have a little hesitation from a UX perspective, since it'll be a little longer, and now the user is probably actively waiting for it to finish instead of having it run from cron. But I'd much rather mimic existing workflows that we know haven't been actively problematic, rather than having to deal with new unknown issues. So let's blow it away! Whoo!

Edit 4: Ah...just noticed a few things. Easiest is that that rm command needs sudo. Next is that the current instruction doesn't mention the dns packages at all, so if we left it we'd have to fix that. But that leads into finally: the existing installation steps are scattered among various steps: "Set up a Python virtual environment", "Install Certbot", and possibly "Install correct DNS plugin". (Which, further thing, here and possibly also in snapd, "Prepare the Certbot command" should probably be after "Install correct DNS plugin".) What are the chances that people just shoved the DNS credentials into the venv, which we've now told them to delete? We could leave the wording as is, saying "repeat all installation instructions." If we keep the --upgrade, it's not so bad to conditionally put in a certbot-dns-x entry if we're on dns instructions. We could also tell people explicitly which steps to repeat; I don't think we should try to take all those contents and shove them into this step. Leaning towards this last one, though it's a little messy as instructions go. So weighing that against the potential pitfalls of just telling people to run pip --upgrade certbot[s]. Ooh we could even extract the things with certbot in the name from pip freeze and pass those back in, instead of having to conditionally include certbot-dns-x, making the command copy-pastable.

Deleting the venv sure does complicate the instructions doesn't it? I've personally changed my mind and think we should tell people to try pip install --upgrade and ideally include certbot-dns-<PLUGIN> in the upgrade command on the wildcard tab. I'm somewhat tempted to try just this without the rm -rf fallback and see how it goes, but I don't have a strong opinion here. If you want to keep it, I think we should update the command to use sudo.

Hm, I think since we've already been using "pip" for Other Linux's uri for some reason, using pip for this might break urls if anyone's bookmarked or posted it somewhere. I like the idea of using other linux () for both of these. I guess since it's python installing from pip is technically from source, but that feels a little weird. Other Linux (pip)? Other Linux (venv)? I think (pip) is pretty analogous to (snapd). Maybe pip-venv for the uri?

EDIT: actually it turns out "Other Linux" is just our current fallthrough; it's just going to whatever we want the default to be. In light of that, I think I'd like to have a specific pip option much in the same we have a specific snapd option, so people can scroll quickly through to find the pip instructions if they're specifically looking for the pip instructions. And then we can still have two Other Linuxes if we want, do we even need a fallthrough? Why do we even have a fallthrough, it's a dropdown, let's just specifically say where those two other linuxes go.

EDIT 2: Or were you thinking get rid of the plain snapd option and just have Other Linux (pip) and Other Linux (snapd)? I wouldn't want to erroneously indicate that those instructions won't work on systems that are listed in the menu

I'm fine with pip-venv for the URI and "Other Linux (pip)" and/or "pip" dropdowns leading to that URI. I'm personally not too worried about the latter because I would like to do a dramatic overhaul of the OS dropdowns in the near future that makes that list much shorter and reduces/eliminates all the redundancy we currently have. Because of this, I think/hope whatever text we choose now is a short term thing.

</p>

<p>If this step leads to errors, run <code>rm -rf /opt/certbot</code> and repeat all installation instructions.</p>
<p>For automatic upgrades, the command in this step can be run by a monthly cron job.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I personally think we should delete this sentence. In our top level discussion, we decided not to give people the command to do this since we're not sure it's reliable enough. If we have concerns about it, I don't think we should suggest it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

</p>
<p>For RPM-based distributions (e.g. Fedora, CentOS ...):
<br>NB1: old distributions will use yum instead of dnf
<br>NB2: RHEL-based distributions use <code>python3X</code> instead of <code>python3</code>
Copy link
Member

Choose a reason for hiding this comment

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

nit: I personally think this "NB#: ..." text from https://certbot.eff.org/docs/contributing.html#running-a-local-copy-of-the-client looks kind of sloppy here. If you agree, can we reformat this maybe as a note with more or less complete sentences? If you disagree, feel free to leave it as is.

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, will give it a try.

@ohemorange
Copy link
Contributor Author

ohemorange commented Mar 3, 2021

I decided to just go with what the dropdown options probably actually should be, despite all that prevaricating. Lmk if you want me to change it to something else.

Also, here's some updated relevant screenshots:

DNS Upgrade
Screenshot from 2021-03-02 16-58-19

Non-DNS Upgrade
Screenshot from 2021-03-02 16-57-06

RHEL install
Screenshot from 2021-03-02 16-51-59

I kept the rm rf there because if someone does hit it, I'd rather have them see that than deal with them coming to ask for help.

Copy link
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!

Thanks again for picking this up.

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.

None yet

3 participants