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

Fix letsencrypt-auto name and long forms of -n #5375

Merged
merged 1 commit into from
Jan 6, 2018

Conversation

bmw
Copy link
Member

@bmw bmw commented Jan 6, 2018

I added NONINTERACTIVE in addition to ASSUME_YES because while we want certbot-auto renew to be non-interactive, I don't want every cron job in the world to rebootstrap without the user's permission. This fixes the long forms --noninteractive and --non-interactive and also makes letsencrypt-auto auto-upgrade as its behavior has always been install things without prompting the user.

I tested this by merging this and #5329 together and making the following changes:

diff --git a/letsencrypt-auto-source/tests/centos6_tests.sh b/letsencrypt-auto-source/tests/centos6_tests.sh
index e3ebbae..9ca8043 100644
--- a/letsencrypt-auto-source/tests/centos6_tests.sh
+++ b/letsencrypt-auto-source/tests/centos6_tests.sh
@@ -25,7 +25,7 @@ if [ $RESULT -ne 0 ]; then
 fi
 
 # bootstrap, but don't install python 3.
-certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade -n > /dev/null 2> /dev/null
+certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade > /dev/null 2> /dev/null
 
 # ensure python 3 isn't installed
 python3 --version 2> /dev/null
@@ -47,8 +47,18 @@ if [ $RESULT -eq 0 ]; then
   exit 1
 fi
 
+cp certbot/letsencrypt-auto-source/letsencrypt-auto ./certbot-auto
+./certbot-auto --no-self-upgrade >/dev/null 2>&1
+# ensure python 3 isn't installed
+python3 --version 2> /dev/null
+RESULT=$?
+if [ $RESULT -eq 0 ]; then
+  error "Python3 is already installed."
+  exit 1
+fi
+
 # bootstrap, this time installing python3
-certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade -n > /dev/null 2> /dev/null
+./certbot-auto --no-self-upgrade --non-interactive > /dev/null 2> /dev/null
 
 # ensure python 3 is installed
 python3 --version > /dev/null

@bmw bmw added this to the 0.21.0 milestone Jan 6, 2018
@bmw bmw requested a review from ohemorange January 6, 2018 02:29
Copy link
Contributor

@ohemorange ohemorange left a comment

Choose a reason for hiding this comment

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

For the test, it seems we'd only want to change:

# bootstrap, this time installing python3
-certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade -n > /dev/null 2> /dev/null
+certbot/letsencrypt-auto-source/letsencrypt-auto --no-self-upgrade --non-interactive > /dev/null 2> /dev/null

Because that's the minimal change to check that this fix worked.

To be clear, what are the cases where we do want to rebootstrap? Could you put down your expectations for the powerset of the following variables?

  • renew/ not renew
  • terminal attached/ no terminal attached
  • non-interactive set/ not set

--noninteractive|--non-interactive|renew)
ASSUME_YES=1;;
--noninteractive|--non-interactive)
NONINTERACTIVE=1;;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're not actually using this flag for anything, we shouldn't set it.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we are leaving this, we should probably also check the short-form.

Copy link
Member Author

Choose a reason for hiding this comment

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

NONINTERACTIVE is the flag we use to determine it's OK to rebootstrap without user intervention. See https://github.com/certbot/certbot/blob/v0.20.0/certbot-auto#L833.

And we do check the short form but due to the fun of POSIX compatible shell scripting, we have to check for the short form differently. See https://github.com/certbot/certbot/blob/v0.20.0/certbot-auto#L83.

@bmw
Copy link
Member Author

bmw commented Jan 6, 2018

For the test, it seems we'd only want to change ...

Sure that's fine. I just quickly added different tests to verify it worked.

To be clear, what are the cases where we do want to rebootstrap? Could you put down your expectations for the powerset of the following variables?

Renew has no affect on whether or not we rebootstrap. For the other variables, if either a terminal is attached or -n/--non-interactive/--noninteractive is set, we'll reboostrap if we need to, otherwise, we will not.

EDIT: The other variable there that's not mentioned is the name of the script (while hacky, this was necessary for improving certbot-auto while keeping backwards compatibility). If the name of the script is letsencrypt-auto, it is equivalent to -n being set.

Copy link
Contributor

@ohemorange ohemorange left a comment

Choose a reason for hiding this comment

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

Ahh, missed that we handle shortform differently. LGTM.

@ohemorange ohemorange merged commit 18f6dea into master Jan 6, 2018
@ohemorange ohemorange deleted the cb-auto-noninteractive branch January 6, 2018 03:27
ohemorange pushed a commit that referenced this pull request Jan 10, 2018
* Use josepy instead of acme.jose. (#5203)

* Parse variables without whitespace separator correctly in CentOS family of distributions (#5318)

* Pin josepy in letsencrypt-auto (#5321)

* pin josepy in le-auto

* Put pinned versions in sorted order

* Pin dependencies in oldest tests (#5316)

* Add tools/merge_requirements.py

* Revert "Fix oldest tests by pinning Google DNS deps (#5000)"

This reverts commit f68fba2.

* Add tools/oldest_constraints.txt

* Remove oldest constraints from tox.ini

* Rename dev constraints file

* Update tools/pip_install.sh

* Update install_and_test.sh

* Fix pip_install.sh

* Don't cat when you can cp

* Add ng-httpsclient to dev constraints for oldest tests

* Bump tested setuptools version

* Update dev_constraints comment

* Better document oldest dependencies

* test against oldest versions we say we require

* Update dev constraints

* Properly handle empty lines

* Update constraints gen in pip_install

* Remove duplicated zope.component

* Reduce pyasn1-modules dependency

* Remove blank line

* pin back google-api-python-client

* pin back uritemplate

* pin josepy for oldest tests

* Undo changes to install_and_test.sh

* Update install_and_test.sh description

* use split instead of partition

* More pip dependency resolution workarounds (#5339)

* remove pyopenssl and six deps

* remove outdated tox.ini dep requirement

* Fix auto_tests on systems with new bootstrappers (#5348)

* Fix pytest on macOS in Travis (#5360)

* Add tools/pytest.sh

* pass TRAVIS through in tox.ini

* Use tools/pytest.sh to run pytest

* Add quiet to pytest.ini

* ignore pytest cache

* print as a string (#5359)

* Use apache2ctl modules for Gentoo systems. (#5349)

* Do not call Apache binary for module reset in cleanup()

* Use apache2ctl modules for Gentoo

* Broader git ignore for pytest cache files (#5361)

Make gitignore take pytest cache directories in to account, even if
they reside in subdirectories.

If pytest is run for a certain module, ie. `pytest certbot-apache` the
cache directory is created under `certbot-apache` directory.

* Fix letsencrypt-auto name and long forms of -n (#5375)

* Deprecate Python2.6 by using Python3 on CentOS/RHEL 6 (#5329)

* If there's no python or there's only python2.6 on red hat systems, install python3

* Always check for python2.6

* address style, documentation, nits

* factor out all initialization code

* fix up python version return value when no python installed

* add no python error and exit

* document DeterminePythonVersion parameters

* build letsencrypt-auto

* close brace

* build leauto

* fix syntax errors

* set USE_PYTHON_3 for all cases

* rip out NOCRASH

* replace NOCRASH, update LE_PYTHON set logic

* use built-in venv for py3

* switch to LE_PYTHON not affecting bootstrap selection and not overwriting LE_PYTHON

* python3ify fetch.py

* get fetch.py working with python2 and 3

* don't verify server certificates in fetch.py HttpsGetter

* Use SSLContext and an environment variable so that our tests continue to never verify server certificates.

* typo

* build

* remove commented out code

* address review comments

* add documentation for YES_FLAG and QUIET_FLAG

* Add tests to centos6 Dockerfile to make sure we install python3 if and only if appropriate to do so.

* Allow non-interactive revocation without deleting certificates (#5386)

* Add --delete-after-revoke flags

* Use delete_after_revoke value

* Add delete_after_revoke unit tests

* Add integration tests for delete-after-revoke.

* Have letsencrypt-auto do a real upgrade in leauto-upgrades option 2 (#5390)

* Make leauto_upgrades do a real upgrade

* Cleanup vars and output

* Sleep until the server is ready

* add simple_http_server.py

* Use a randomly assigned port

* s/realpath/readlink

* wait for server before getting port

* s/localhost/all interfaces

* update Apache ciphersuites (#5383)

* Fix macOS builds for Python2.7 in Travis (#5378)

* Add OSX Python2 tests

* Make sure python2 is originating from homebrew on macOS

* Upgrade the already installed python2 instead of trying to reinstall
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.

None yet

2 participants