-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR] Extract ApplianceConsole and fixtures cli methods into cfme.utils.appliance.console #9632
Conversation
import socket | ||
import tempfile | ||
from contextlib import contextmanager | ||
from re import escape as resc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't really like renaming this import, thoughts @ManageIQ/qe-committers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that it seems like some "rescue" would be done. I was not able to find out better name that would still be short for often use. Therefore I suggest using just esc()
or just r()
?
Hopefully native English speakers will have solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ree()
, e()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just import re
, and call re.escape
The reconfigure_primary_replication_node is in the appliance_console attribute of the appliance, not directly in appliance.
I detected some fixture changes in commit 3adba49 The global fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
interaction.send('ap') | ||
interaction.answer('Press any key to continue.', '', timeout=20) | ||
interaction.answer('Choose the advanced setting: ', | ||
'6' if self.appliance.version < '5.10' else '8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional since it doesn't change functionality, but we can remove this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umn. Quite a good point, but I am also doing upgrade testing and there are some not-automated test doing upgrades on ha setup. There is test_upgrade_multi_ha_inplace
which needs to be automated and once I do that, it certainly will use this method and the condition will come handy.
Nice work Jaro!! |
Yeah. Especially when the result is 12 lines shorter than the original. |
…e.utils.appliance.console (ManageIQ#9632) * Extract ApplianceConsole and ApplianceConsoleCli to a plugin. * Extract non-fixtures from fixtures/cli * Resolve circular imports * Fix option configuring the failover monitor. * Fix Attribute errr: self.appl -> self.appliance * Fix AttributeError caused by missing appliance_console The reconfigure_primary_replication_node is in the appliance_console attribute of the appliance, not directly in appliance.
Purpose or Intent
Hopefully an enhancement of how the code is layed out in the repo.
{ py.test: -v cfme/tests/cli/test_appliance_console_db_restore.py }