disable: cli disable progress refactor #3028
Conversation
|
Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference) GitHub Issues: No GitHub issues are fixed by this PR. (No commits have Fixes: #### references) Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references) Documentation: The changes in this PR do not require documentation changes. 👍 this comment to confirm that this is correct. |
|
🌎 This PR changes translatable messages. 🌏 Please select which scenarios apply. For further explanation, please read our policy on message changes.
|
orndorffgrant
force-pushed
the
cli-disable-progress-refactor
branch
9 times, most recently
from
50aa367
to
5f02aff
Compare
orndorffgrant
changed the title
orndorffgrant
force-pushed
the
cli-disable-progress-refactor
branch
from
5f02aff
to
20c41bf
Compare
|
|
||
| def get_title(cfg: UAConfig, ent_name: str, variant=""): | ||
| try: | ||
| return entitlement_factory(cfg, ent_name, variant=variant)( |
There was a problem hiding this comment.
Just to double check, what is the advantage of this approach ? We are now creating the entitlement object to fetch the title, which is not necessary.
Is that because of we might miss a variant title by using ENTITLEMENT_NAME_TO_TITLE ?
There was a problem hiding this comment.
It is unfortunately necessary for cis, because the title depends on self.called_name
There was a problem hiding this comment.
I cry so much everytime this kind of comment pops out somewhere
We need to kill this horrible bug
| @@ -216,7 +186,9 @@ def action_enable(args, *, cfg, **kwargs) -> int: | |||
| if json_output: | |||
| progress = api.ProgressWrapper() | |||
| else: | |||
| progress = api.ProgressWrapper(CLIEnableProgress()) | |||
| progress = api.ProgressWrapper( | |||
There was a problem hiding this comment.
Even though this is not a problem introduced in this PR, but I noticed that we are now creating the same entitlement object on lines 155 and 193. I think we should only do that once
There was a problem hiding this comment.
Yep I can consolidate them
uaclient/cli/cli_util.py
Outdated
| def _on_event(self, event: str, payload): | ||
| if event == "info": | ||
| print(payload) | ||
| return |
There was a problem hiding this comment.
Just a nit, but I think we can remove the empty return here
orndorffgrant
force-pushed
the
cli-disable-progress-refactor
branch
from
20c41bf
to
1ec0a28
Compare
|
@lucasmoura updated! |
the title depends on the called_name and variant, so getting it from the class is not sufficient.
orndorffgrant
force-pushed
the
cli-disable-progress-refactor
branch
from
afb4727
to
de1856d
Compare
Why is this needed?
This refactors the disable cli command to use the progress api for printing messages to the user, which will allow the api for disable to be implemented without hacking around prompts deep in the call stack.
Test Steps
CI and tests should all pass
Checklist
Does this PR require extra reviews?