Add check for HostGAPlugin version #2413
Conversation
narrieta
requested review from
dhivyaganesan,
kevinclark19a,
larohra,
nagworld9 and
ZhidongPeng
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #2413 +/- ##
===========================================
+ Coverage 71.61% 71.63% +0.02%
===========================================
Files 100 100
Lines 14610 14633 +23
Branches 2087 2090 +3
===========================================
+ Hits 10463 10483 +20
- Misses 3683 3687 +4
+ Partials 464 463 -1
Continue to review full report at Codecov.
|
| self._text = json_text | ||
| self._host_ga_plugin_version = FlexibleVersion() | ||
| self._schema_version = FlexibleVersion() |
There was a problem hiding this comment.
Can you please help me understand what FlexibleVersion() without any parameter passed returns and what will be the default value of _host_ga_plugin_version?
Had a quick read about FlexibleVersion(version.Version) and looks like it expects a version.
There was a problem hiding this comment.
self.version is intialized to empty and each property defaults to 0, as in
def major(self):
return self.version[0] if len(self.version) > 0 else 0
| @@ -108,8 +109,10 @@ def compare_attribute(attribute): | |||
| compare_attribute("activity_id") | |||
| compare_attribute("correlation_id") | |||
| compare_attribute("created_on_timestamp") | |||
| compare_attribute("status_upload_blob") | |||
| compare_attribute("status_upload_blob_type") | |||
| # The status blob was added after version 112 | |||
There was a problem hiding this comment.
I'm guessing this check is temporary until this feature is stabilized? If so, could you please add a comment about it too. Thanks!
There was a problem hiding this comment.
On that note, do the latest version of HostGA contain all the attributes that we're parsing? Or will we have to have a similar check like this for other versions too in the future?
There was a problem hiding this comment.
1 more question, what's the min version where the HostGA version is available? Is it .112 or greater than that?
There was a problem hiding this comment.
-
I'm guessing this check is temporary until this feature is stabilized? If so, could you please add a comment about it too. Thanks!
It will stay there for a while, probably it can be removed once we get telemetry indicating that there are no old HostGAPlugins in the wild. For the moment this check is there just to avoid generating noise. No harm if this is not removed so I'll leave it as is -
On that note, do the latest version of HostGA contain all the attributes that we're parsing? Or will we have to have a similar check like this for other versions too in the future?
We will have additional checks on the version. The purpose of this PR is mainly to set the infrastructure needed to perform those checks.
- 1 more question, what's the min version where the HostGA version is available? Is it .112 or greater than that?
Not sure of the exact version, but Flexible version defaults to 0.0.0.0 so version 0 indicates the version field is not there
There was a problem hiding this comment.
We will have additional checks on the version. The purpose of this PR is mainly to set the infrastructure needed to perform those checks.
I am personally not a big fan of putting checks on versions because they can be never ending. But at the same time I'm not sure of a better way to do it either so approved the PR
| # | ||
| # 112 and below should not check the status blob | ||
| # | ||
| from_vm_settings._host_ga_plugin_version = FlexibleVersion("1.0.8.112") |
There was a problem hiding this comment.
Maybe worth adding a check where we dont get any HostGA version at all too?
There was a problem hiding this comment.
didn't consider it interesting, since flexible version inits to 0
| @@ -1,4 +1,6 @@ | |||
| { | |||
| "hostGAPluginVersion":"1.0.8.115", | |||
| "vmSettingsSchemaVersion":"0.0", | |||
There was a problem hiding this comment.
Just to verify, these fields have always been present in real VmSettings objects, right?
Edit: Or, at least, since host ga plugin v112?
There was a problem hiding this comment.
They were added post 112, not sure when. We default to 0.0.0.0 if not present
narrieta
dismissed stale reviews from kevinclark19a and dhivyaganesan
via
320e4a6
Some of the older versions of the HostGAPlugin do not include the status blob; added check to skip it when comparing against ExtensionsConfig.
Also added telemetry for the HostGAPlugin version.