Add data-plane pre upgrade sanity checks #222
Conversation
- create a new NamedTuple class to be the COU Namespace. With that, type is still checked and few arguments are necessary to pass to the functions that use the arguments passed by the user. Moreover, docstrings are shorter and easier to maintain - Global variables for "data-plane" and "control-plane".
- check if control-plane is already upgraded if user pass "data-pane" upgrade group - check if data-plane apps exists if user pass "data-pane" upgrade group - splitted the logic on determine_upgrade_target to several functions
gabrielcocenza
requested review from
Pjack,
aieri,
agileshaw,
jneo8 and
rgildein
- fixed run_upgrade unit test
- added expected error messages on the remaining sanaity checks unit tests.
gabrielcocenza
changed the title
- small change in is_highest_release_achieved function
cou/steps/plan.py
Outdated
| is_data_plane_ready_to_upgrade(analysis_result) | ||
|
|
||
|
|
||
| def is_valid_openstack_cloud(analysis_result: Analysis) -> None: |
There was a problem hiding this comment.
Naming suggests this should return a boolean (is_*). Perhaps assert_* or verify_*?
| @@ -53,6 +54,147 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| def pre_plan_sanity_checks(args: CLIargs, analysis_result: Analysis) -> None: | |||
| """Pre checks to generate the upgrade plan. | |||
There was a problem hiding this comment.
It should be documented that this works by raising a particular set of exceptions if a check fails.
There was a problem hiding this comment.
I see little benefit on doing this. In Java for example, the checked exception forced the signature of every method would list all of the exceptions that it could pass to its caller and has a high price. If we start getting deep into this we could have a situation where we would need to go three or more levels checking all the exceptions that can happen.
On theory a change at a low level of the software would imply on updating on the higher level and I don't think it worth.
If you have the Clean Code book, chapter 7 has a session talking about this that worth the reading.
There was a problem hiding this comment.
@samuelengineer that's good point, I lost myself with warlock operators in this PR.
@gabrielcocenza I see your point, however all those verify_* function are simple helper functions, and we expected this function to raise those exceptions. If I look at the function name pre_plan_sanity_checks, and then the description, I could not tell what this function actually do? It's not returning anything and not mention raising anything.
Please stop referring to that book (I'm going to buy and read it, because there could be good point). I believe that you're misinterpreting things from it, or it has written not for Python. Thanks
a boolean. - removed the pre_plan_sanity_checks is_valid_openstack_cloud to be nested under determine_upgrade_target - fixed unit tests to test the verify_* functions instead of the pre_plan_sanity_checks
| @@ -53,6 +54,147 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| def pre_plan_sanity_checks(args: CLIargs, analysis_result: Analysis) -> None: | |||
| """Pre checks to generate the upgrade plan. | |||
There was a problem hiding this comment.
@samuelengineer that's good point, I lost myself with warlock operators in this PR.
@gabrielcocenza I see your point, however all those verify_* function are simple helper functions, and we expected this function to raise those exceptions. If I look at the function name pre_plan_sanity_checks, and then the description, I could not tell what this function actually do? It's not returning anything and not mention raising anything.
Please stop referring to that book (I'm going to buy and read it, because there could be good point). I believe that you're misinterpreting things from it, or it has written not for Python. Thanks
This PR should be merged after #221