Refactor config #288
Refactor config #288
Conversation
|
No test code added yet. Additional test code will be added once there is agreement on whether the approach and implementation are acceptable. |
sputnik13
force-pushed
the
refactor_config
branch
from
2c12b3d to
f4ced0e
Compare
There was a problem hiding this comment.
This is a really nice refactoring. I like the @ansible_play_var decorators, their use makes the code in config/ec2.py and config/azure.py very explicit about which functions will be used for ansible. It took me a bit to follow the indirection with in the implementation of the decorators for the play variables (I had to learn some python along the way).
I have not tried running this yet, I plan to do that in a bit.
| return True | ||
| return False | ||
|
|
||
| # test method, might want to make private or just move to test module |
There was a problem hiding this comment.
Would this be easy to do?
There was a problem hiding this comment.
yes it should be pretty straight forward, especially since python has no notion of variable hiding or protection, test code can access and/or modify any internal variables just fine. Making private in python land is adding underscore prefix(es) and is more a convention than a rule.
sputnik13
force-pushed
the
refactor_config
branch
2 times, most recently
from
b130bb9 to
b452b8d
Compare
|
@keith-turner I've addressed the outstanding comments, a follow-up commit with unit tests should be coming a bit later |
@sputnik13 were you still planning on making more changes? If not, I think this may be ready to merge. |
|
@keith-turner I was planning on adding unit tests for the new code but am bogged down with an urgent project. If you want to merge and have a separate follow up for the unit tests that would be fine with me |
- split config.py to DeployConfig base class and Ec2DeployConfig, ExistingDeployConfig, and AzureDeployConfig subclasses - add @AbstractMethod annotations for common interfaces to DeployConfig base class - move target specific code to respective subclasses
- add python decorators to annotate config items to allow generic handling of configs - moved some ec2 specific configs to Ec2DeployConfig
- integrate default value handling for host_var and play_var in to base config class accessor for respective var dictionaries - remove use of 'azure_vars' and consolidate as host_vars
- made config.shutdown_delay_minutes not abstract so existing and azure cluster types wouldn't have to implement it - changed _ansible_vars dictionary values a list of structs rather than tuples so that each field has an understandable name - removed instance_tags from base config
sputnik13
force-pushed
the
refactor_config
branch
from
b452b8d to
ad45d3f
Compare
2 participants
This is a refactor of config.py to model the difference in configurations based on the cluster type [existing, ec2, azure] by creating an object hierarchy for the configs with shared facilities/attributes in the base class and cluster type specific attributes in the cluster specific implementation.
Also adds python decorators to help annotate where the config item(s) should be rendered to, if it needs to be rendered, and removed most of the related logic from muchos/existing.py.
Lastly, split off the test code for each config types in to separate files based on cluster type.
Implements: #285