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

Revamp register subcommand #3816

Closed
cowlicks opened this issue Nov 22, 2016 · 5 comments
Closed

Revamp register subcommand #3816

cowlicks opened this issue Nov 22, 2016 · 5 comments

Comments

@cowlicks
Copy link
Contributor

The cli subcommand register is currently used for account management commands. This causes weird spellings like certbot register --deactivate. So register should probably be changed to account. I did some preliminary work on this here. But missed things like cli flags (--update-registration) and documentation.

@cpu
Copy link
Contributor

cpu commented Nov 22, 2016

Just wanted to mention that providing the "account" verb in the CLI going forward also has good synergy with the decision to rename the "registration" in ACME to "account". When the spec is finalized it will be using "account".

@pde
Copy link
Member

pde commented Dec 3, 2016

Go ahead and make a PR! The old "auth" verb is now "certonly", so you can use that as a model.

@bmw
Copy link
Member

bmw commented Jan 3, 2017

We had a meeting about the Certbot command line awhile ago and decided we should make some changes here. In general, we decided that we should favor add more verbs/subcommands (which can be nicely grouped in our help as shown in #3883) rather than more flags to change behaviors of existing subcommands.

What this means here is that:

  • certbot register --update-registration should become its own subcommand. I'm open to suggestions on the name here. To implement this, we can probably change the verb in certbot.cli.HelpfulArgumentParser.parse_args if certbot register --update-registration is used to preserve compatibility.
  • certbot register --deactivate as implemented in Impelment account deactivation #3571 should become something like certbot unregister.
  • certbot register should only be used to create a new account with the ACME CA. We can potentially change the name of this subcommand but I personally like it given its new, more specific purpose.

@bmw bmw changed the title Alias "account" for "register" in the cli Revamp register subcommand Jan 3, 2017
@bmw bmw removed the easy-fix label Jan 3, 2017
@bmw bmw added this to the 0.11.0 milestone Jan 3, 2017
@bmw bmw modified the milestones: 0.12.0, 0.11.0 Jan 18, 2017
@bmw bmw modified the milestones: 0.12.0, 0.13.0, 0.14.0 Mar 2, 2017
@ohemorange ohemorange modified the milestones: 0.14.0, 1.0.0 Apr 13, 2017
@dschlessman
Copy link
Contributor

I'm taking a look at this one at PyCon.

@bmw
Copy link
Member

bmw commented May 15, 2018

Here's a way to run a single problematic test with pytest:

pytest certbot/tests/main_test.py::MainTest::test_update_registration_unsafely -s

-s stops pytest from capturing terminal output.

On top of that, our tests were capturing terminal output. This breaks the test, but so you can step through it to see what's going on, you can make the following changes:

diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py
index 22653ca3..c0a5b83f 100644
--- a/certbot/tests/main_test.py
+++ b/certbot/tests/main_test.py
@@ -624,10 +624,10 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         args = self.standard_args + args
 
         toy_stdout = stdout if stdout else six.StringIO()
-        with mock.patch('certbot.main.sys.stdout', new=toy_stdout):
-            with mock.patch('certbot.main.sys.stderr') as stderr:
-                ret = main.main(args[:])  # NOTE: parser can alter its args!
-        return ret, toy_stdout, stderr
+        #with mock.patch('certbot.main.sys.stdout', new=toy_stdout):
+        #    with mock.patch('certbot.main.sys.stderr') as stderr:
+        main.main(args[:])  # NOTE: parser can alter its args!
+        return None, toy_stdout, stderr
 
     def test_no_flags(self):
         with mock.patch('certbot.main.run') as mock_run:

Use https://stackoverflow.com/questions/12320863/how-do-you-take-a-git-diff-file-and-apply-it-to-a-local-branch-that-is-a-copy-o if you want to apply this diff with git.

To set a breakpoint, use import ipdb; ipdb.set_trace().

ohemorange pushed a commit that referenced this issue Dec 12, 2018
* address issue #3816

* formatting update

* remove unused variable

* address pylint trailing whitespace error

* revert whitespace add

* update boulder ci test for new update_registration verb

* address code review comments

* Issue 3816: Revert renaming '...update_regristration...' tests to '...update_account...'. Fix removing update_registration default argument value.

* Issue 3816: Fix '--update-registration' not referring to 'update_registration' default as opposed to 'update_account'.

* Issue 3816: delint tox output.

* Issue 3816: Change @example.org domain to @domain.org in boulder test script

* Issue 3816: Update CHANGELOG.md for Issue 3816 and remove extraneous space in main.py

* Issue 3816: Remove extraneous default variable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants