feat: have landscape-config --silent only send a registration messa…
#258
Conversation
…ge when required and add a new `--force-registration` flag
|
Currently, Manual testing instructions:
|
|
Why do we want |
| @@ -2756,6 +2804,13 @@ def test_success_means_exit_code_0(self): | |||
| result = determine_exit_code("success") | |||
| self.assertEqual(0, result) | |||
|
|
|||
| def test_registration_skipped_means_exit_code_0(self): | |||
| """ | |||
| When passed "success" the determine_exit_code function returns 0. | |||
There was a problem hiding this comment.
Nit: looks like a carryover from a copy-paste; should be "registration-skipped"
There was a problem hiding this comment.
Should be "registration-skipped"?
| if config.is_registered: | ||
|
|
||
| registration_status = is_registered(config) |
There was a problem hiding this comment.
Nit: could we just replace usages of registration_status with already_registered?
There was a problem hiding this comment.
I think I picked this nit.
| set_secure_id(config, "registering") | ||
| secure_id = get_secure_id(config) | ||
| if (not secure_id) or config.force_registration: | ||
| set_secure_id(config, "registering") |
There was a problem hiding this comment.
I realize this is somewhat of a carryover from the existing implementation, but is it necessary to set the secure_id in the configuration file and in the registration handler? Could we get rid of it here since you set it in the registration handler now?
|
How is this different from this pull request #243 |
|
One other question. Will this break existing scripts that rely on landscape-config sending a new registration as the default behavior? I can see that the charm will be affected by this. For example, if the user runs the re-register run action, then that is implement simply by a |
We don't. I made a copy-pasta error in my testing instructions. Editing them to better reflect the intentions. |
The |
Yes, it sounds like this change will break that behavior. It can be adjusted in the charm by calling |
|
In that case I would prefer we have a flag |
|
OK, good point. I'll fix this up and let Carlos know. |
|
Leaning towards |
|
Updated, restoring the behavior of |
| @@ -2756,6 +2804,13 @@ def test_success_means_exit_code_0(self): | |||
| result = determine_exit_code("success") | |||
| self.assertEqual(0, result) | |||
|
|
|||
| def test_registration_skipped_means_exit_code_0(self): | |||
| """ | |||
| When passed "success" the determine_exit_code function returns 0. | |||
There was a problem hiding this comment.
Should be "registration-skipped"?
| default=default_answer, | ||
| ) | ||
|
|
||
| if should_register or config.silent: |
There was a problem hiding this comment.
I think this check is causing the client to reset the secure_id even if we pass --register-if-needed and the client is already registered; the restart_client function sets the secure_id to 'registering", and then we end up with another pending computer.
|
Updated. For manual testing now:
|
| if config.silent: | ||
| should_register = False | ||
|
|
||
| if config.force_registration: |
There was a problem hiding this comment.
nit: you could rearrange this block to make it slightly simpler, if you so desire:
if config.force_registration:
should_register = True
elif config.register_if_needed:
should_register = not already_registered
elif config.silent:
should_register = True
else:
...
…ge when required and add a new
--force-registrationflag