Set up supporting code to compare ExtensionsConfig with vmSettings #2378
Conversation
narrieta
requested review from
dhivyaganesan,
kevinclark19a,
larohra,
nagworld9 and
ZhidongPeng
as code owners
| @@ -82,13 +82,6 @@ def __init__(self): | |||
| self.vmAgentManifests = DataContractList(VMAgentManifest) | |||
|
|
|||
|
|
|||
| class RequiredFeature(DataContract): | |||
There was a problem hiding this comment.
vmSettings does not expose a Value. Since it is not used anyway, I simplified the code to use simple strings instead of instances of RequiredFeature
| if force_update or self._goal_state is None or self._goal_state.incarnation != goal_state.incarnation: | ||
| if force_update or \ | ||
| self._goal_state is None or self._extensions_goal_state is None or \ | ||
| self._goal_state.incarnation != goal_state.incarnation: |
There was a problem hiding this comment.
I added a check on self._extensions_goal_state is None. Checking only on self._goal_state is not enough since an exception may leave the latter initialized and the former uninitialized.
Codecov Report
@@ Coverage Diff @@
## develop #2378 +/- ##
===========================================
+ Coverage 70.89% 70.99% +0.10%
===========================================
Files 97 97
Lines 14107 14142 +35
Branches 2029 2034 +5
===========================================
+ Hits 10001 10040 +39
+ Misses 3657 3654 -3
+ Partials 449 448 -1
Continue to review full report at Codecov.
|
| self._save_cache(redacted_vm_settings_text, VM_SETTINGS_FILE_NAME.format(self._vm_settings_etag)) | ||
| if self._extensions_goal_state is not None: | ||
| # disable=assignment-from-none: E1128: Assigning result of a function call, where the function returns None | ||
| text = self._extensions_goal_state.get_redacted_text() # pylint: disable=assignment-from-none |
There was a problem hiding this comment.
get_redacted_text() can return None; suppressing warning
narrieta
changed the title
| @staticmethod | ||
| def compare(from_extensions_config, from_vm_settings): | ||
| """ | ||
| Compares the current instance against 'other' and logs a GoalStateMismatch message if they are different |
There was a problem hiding this comment.
NIT: The comment suggests that the self will be compared with other but the function definition is a static class comparing 2 different arguments.
Maybe modify the comment a bit to make it a bit more clearer?
There was a problem hiding this comment.
thanks, initially this was a member function and i made it static without updating the comment; will do that
| super(_ExtensionsGoalStateFromExtensionsConfig, self).__init__() | ||
| self._id = incarnation | ||
| self._text = xml_text | ||
| self._type = 'ExtensionsConfig' |
There was a problem hiding this comment.
Maybe make the type an Enum - ExtensionsConfig vs VmSettings?
There was a problem hiding this comment.
I may removed this. I mean to use for logging purposes, but the name of the class is already pretty descriptive so I think I'll just use that
| if required_features is not None: | ||
| if not isinstance(required_features, list): | ||
| raise Exception("requiredFeatures should be an array") | ||
| self.required_features.append(required_features) |
There was a problem hiding this comment.
Shouldn't this be extend over append?
| ExtensionsConfig when the goal state is retrieved from the WireServe or from vmSettings when it is retrieved from | ||
| the HostGAPlugin. | ||
|
|
||
| NOTE: Do not instantiate this class directly, use one of the create_* methods instead. |
There was a problem hiding this comment.
Would it be worth making an interface and providing the create classes there? i.e. making this a private class so that we don't have to rely on this docstring?
There was a problem hiding this comment.
not sure i follow your suggestion, how would you express that in code?
| @@ -1204,6 +1204,42 @@ def test_update_goal_state_should_not_persist_the_protected_settings(self): | |||
| len(protected_settings), | |||
| "Could not find the expected number of redacted settings. Expected {0}.\n{1}".format(len(protected_settings), extensions_config)) | |||
|
|
|||
| def test_update_goal_state_should_save_goal_state(self): | |||
There was a problem hiding this comment.
moved from test_goal_state.py
| @@ -31,71 +20,3 @@ def test_fetch_goal_state_should_raise_on_incomplete_goal_state(self, _): | |||
| GoalState(protocol.client) | |||
| self.assertEqual(_NUM_GS_FETCH_RETRIES, mock_sleep.call_count, "Unexpected number of retries") | |||
|
|
|||
| def test_update_goal_state_should_save_goal_state(self, _): | |||
| for e in extensions: | ||
| self.assertEqual(e["settings"][0]["protectedSettings"], "*** REDACTED ***", "The protected settings for {0} were not redacted".format(e["name"])) | ||
|
|
||
| def test_update_vm_settings_should_should_retry_on_resource_gone_error(self, _): |
There was a problem hiding this comment.
moved to test_extensions_goal_state.py
|
|
||
|
|
||
| class ExtensionsGoalStateTestCase(HttpRequestPredicates, AgentTestCase): | ||
| def test_fetch_vm_settings_should_should_retry_on_resource_gone_error(self): |
There was a problem hiding this comment.
moved from test_goal_state.py
| self.assertEqual("GET_VM_SETTINGS_TEST_CONTAINER_ID", request_headers[1][hostplugin._HEADER_CONTAINER_ID], "The retry request did not include the expected header for the ContainerId") | ||
| self.assertEqual("GET_VM_SETTINGS_TEST_ROLE_CONFIG_NAME", request_headers[1][hostplugin._HEADER_HOST_CONFIG_NAME], "The retry request did not include the expected header for the RoleConfigName") | ||
|
|
||
| def test_compare_should_report_mismatches_between_extensions_config_and_vm_settings(self): |
|
|
||
| def test_compare_should_report_mismatches_between_extensions_config_and_vm_settings(self): | ||
| from_vm_settings = ExtensionsGoalState.create_from_vm_settings("123", fileutil.read_file(os.path.join(data_dir, "hostgaplugin/vm_settings.json"))) | ||
| from_extensions_config = ExtensionsGoalState.create_from_vm_settings("123", fileutil.read_file(os.path.join(data_dir, "hostgaplugin/vm_settings.json"))) |
There was a problem hiding this comment.
Shouldn't this be from extension config?
There was a problem hiding this comment.
we should have a jar and put 1 dollar for each darn copy and paste bug
then have a big party on each release
This PR prepares the code to compare ExtensionsConfig with vmSettings.
For now, it only compares the RequiredFeatures; subsequent PRs will parse and compare other attributes.