Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #634: Added value-mapped string to properties in instance table output #636

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

andy-maier
Copy link
Contributor

@andy-maier andy-maier commented Jun 20, 2020

Update: For latest status, see the last comments of this PR

Ready for initial use and review.

The use of valuemap/values is always enabled when displaying instances in a table format and when the WBEM server supports class operations, so there is no option to enable or disable it.
The properties that are integer-typed and that have a valuemapping defined or inherited in the creation class of the instance are displayed with the Values value in parenthesis after the integer value.

Example with the changes from this PR, that added a "gender" property with value-mapped values for male and female:

$ pywbemcli -m tests/unit/simple_assoc_mock_model.mof -o table instance enumerate TST_Person
Instances: TST_Person
+------------+------------+------------------+-----------+
| name       | gender     | secondProperty   | counter   |
|------------+------------+------------------+-----------|
| "Gabi"     | 1 (female) |                  |           |
| "Mike"     | 2 (male)   |                  |           |
| "Saara"    | 1 (female) |                  |           |
| "Sofi"     | 1 (female) |                  |           |
| "Gabisub"  | 1 (female) | "four"           | 4         |
| "Mikesub"  | 2 (male)   | "one"            | 1         |
| "Saarasub" | 1 (female) | "two"            | 2         |
| "Sofisub"  | 1 (female) | "three"          | 3         |
+------------+------------+------------------+-----------+

Discussion points:

  • Do we need an option to enable or disable the value-mapping output?
  • If the WBEM server does not support class operations, do we expect a warning or just silently not showing value-mapping output?
  • Do we also need it for string-typed properties?
    Mike has provided his views on these discussion points further down.

Limitation: This PR limits the support to scalar elements. Array elements are displayed as normal, i.e. without the value-mapped value in parenthesis.

@andy-maier andy-maier self-assigned this Jun 20, 2020
@andy-maier andy-maier added this to the 0.7.0 milestone Jun 20, 2020
@coveralls
Copy link

coveralls commented Jun 20, 2020

Coverage Status

Coverage increased (+0.05%) to 90.65% when pulling e35fdab on andy/valuemap into 7a7b414 on master.

@andy-maier andy-maier force-pushed the andy/valuemap branch 2 times, most recently from 2da7adc to 885c981 Compare June 21, 2020 14:29
@andy-maier andy-maier changed the title [WIP] Added ValueMap value to instance table output Added ValueMap value to instance table output Jun 21, 2020
@FarmerMike252
Copy link

On your discussion points I would say:

  • Do we need an option to enable or disable the value-mapping output? NO
  • If the WBEM server does not support class operations, do we expect a warning or just silently not showing value-mapping output? JUST SILENTLY SHOW VALUEMAP
  • Do we also need it for string-typed properties? NO

@andy-maier
Copy link
Contributor Author

andy-maier commented Jun 23, 2020

@FarmerMike252 In your answer to the second question, did you actually mean to say "JUST SILENTLY NOT SHOW VALUEMAP"?

If so, then the PR that is up is in tune with your answers (although I would say we should test against SFCB to be really sure)

@andy-maier andy-maier changed the title Added ValueMap value to instance table output Fixes #634: Added ValueMap value to instance table output Jun 23, 2020
@KSchopmeyer
Copy link
Contributor

I was thinking of just the values rather than values and maps. with option to show the maps or maps and values. I don't have strong reason however since this is all new

@FarmerMike252
Copy link

FarmerMike252 commented Jun 23, 2020

I am fine with just showing the value/valuemap pair. However, my testing shows that it only works sometimes. It worked on CIM_RegisteredProfile instances, but when I tested it on CIM_ComputerSystem the translation was only done on EnabledState.
My test output is:

pywbemcli> instance enumerate CIM_ComputerSystem --pl InstanceID,CreationClassName,Name,Dedicated,OperationalStatus,HealthState,EnabledS
tate
Instances: CIM_ComputerSystem
+--------------+-------------+----------------------+----------------------+--------------------+---------------------+
| InstanceID   | Dedicated   | CreationClassName    | Name                 | EnabledState       | OperationalStatus   |
|--------------+-------------+----------------------+----------------------+--------------------+---------------------|
| "CS0"        | 3, 15       | "CIM_ComputerSystem" | "ACME+CF2A509130008" | 5 (Not Applicable) | 2, 32769            |
|              |             |                      | "9"                  |                    |                     |
| "CS_A"       | 12          | "CIM_ComputerSystem" | "ACME+CF2A509130008" | 2 (Enabled)        | 2, 32769            |
|              |             |                      | "9+SP_A"             |                    |                     |
| "CS_B"       | 12          | "CIM_ComputerSystem" | "ACME+CF2A509130008" | 2 (Enabled)        | 2, 32769            |
|              |             |                      | "9+SP_B"             |                    |                     |
+--------------+-------------+----------------------+----------------------+--------------------+---------------------+

pywbemcli> instance enumerate CIM_ComputerSystem --pl InstanceID,CreationClassName,Name,Dedicated,OperationalStatus
Instances: CIM_ComputerSystem
+--------------+-------------+----------------------+---------------------------+---------------------+
| InstanceID   | Dedicated   | CreationClassName    | Name                      | OperationalStatus   |
|--------------+-------------+----------------------+---------------------------+---------------------|
| "CS0"        | 3, 15       | "CIM_ComputerSystem" | "ACME+CF2A5091300089"     | 2, 32769            |
| "CS_A"       | 12          | "CIM_ComputerSystem" | "ACME+CF2A5091300089+SP_" | 2, 32769            |
|              |             |                      | "A"                       |                     |
| "CS_B"       | 12          | "CIM_ComputerSystem" | "ACME+CF2A5091300089+SP_" | 2, 32769            |
|              |             |                      | "B"                       |                     |
+--------------+-------------+----------------------+---------------------------+---------------------+

pywbemcli> instance enumerate CIM_ComputerSystem --pl InstanceID,CreationClassName,Name,Dedicated
Instances: CIM_ComputerSystem
+--------------+-------------+----------------------+----------------------------+
| InstanceID   | Dedicated   | CreationClassName    | Name                       |
|--------------+-------------+----------------------+----------------------------|
| "CS0"        | 3, 15       | "CIM_ComputerSystem" | "ACME+CF2A5091300089"      |
| "CS_A"       | 12          | "CIM_ComputerSystem" | "ACME+CF2A5091300089+SP_A" |
| "CS_B"       | 12          | "CIM_ComputerSystem" | "ACME+CF2A5091300089+SP_B" |
+--------------+-------------+----------------------+----------------------------+
pywbemcli>

@FarmerMike252
Copy link

Andy, yes I meant "JUST SILENTLY NOT SHOW VALUEMAP". However, look at my previous comment. I don't understand why we are not getting the output on Dedicated and OperationalStatus.

@andy-maier
Copy link
Contributor Author

andy-maier commented Jun 25, 2020

I'll look into it. It is definitely not intended :-) The Dedicated and OperationalStatus properties are both arrays, maybe there is a bug with arrays.

@andy-maier
Copy link
Contributor Author

andy-maier commented Jun 25, 2020

It turns out that pywbem.ValueMapping only supports scalar integer-typed properties: https://github.com/pywbem/pywbem/blob/master/pywbem/_valuemapping.py#L714.

The reason this did not fail with the array properties, is that in my use of it in pywbemtools, I was unintendedly consistent with that, by using a type check that did not match for arrays: https://github.com/pywbem/pywbemtools/blob/andy/valuemap/pywbemtools/pywbemcli/_common.py#L1469

The action is to add support for array elements to pywbem.ValueMapping, and to accept arrays in the code in pywbemtools. It seems to me that that will cause pywbemtools to have pywbem 1.0.0 as a minimum version.

I have created pywbem issues:

@andy-maier andy-maier changed the title Fixes #634: Added ValueMap value to instance table output Partly fixes #634: Added ValueMap value to instance table output (scalar elements only) Jun 25, 2020
@andy-maier andy-maier changed the title Partly fixes #634: Added ValueMap value to instance table output (scalar elements only) Fixes #634: Added ValueMap value to instance table output (scalar and array elements) Jun 27, 2020
@andy-maier
Copy link
Contributor Author

andy-maier commented Jun 27, 2020

It turned out that pywbem.ValueMapping did support array-typed CIM elements all along, it was just not documented. What was missing was an ability of ValueMapping.tovalues() to pass in a list or tuple of element values in a single call, so that a caller would not need to distinguish the caes of scalar and array typed elements. This ability will be added in pywbem 0.17.3 and 1.0.0b2. However, pywbemtools does not depend on this new ability because it processes one property at a time, and thus always invokes ValueMapping.tovalues() with a single value (for both the scalar-typed and array-typed elements).

As a result, this PR now works for both scalar-typed and array-typed properties, for all supported versions of pywbem:

$ pywbemcli -m tests/unit/simple_assoc_mock_model.mof -o table instance enumerate TST_Person
Instances: TST_Person
+------------+------------+-----------------------+------------------+-----------+
| name       | gender     | likes                 | secondProperty   | counter   |
|------------+------------+-----------------------+------------------+-----------|
| "Gabi"     | 1 (female) |                       |                  |           |
| "Mike"     | 2 (male)   | 1 (books), 2 (movies) |                  |           |
| "Saara"    | 1 (female) | 1 (books)             |                  |           |
| "Sofi"     | 1 (female) |                       |                  |           |
| "Gabisub"  | 1 (female) |                       | "four"           | 4         |
| "Mikesub"  | 2 (male)   | 1 (books), 2 (movies) | "one"            | 1         |
| "Saarasub" | 1 (female) | 2 (movies)            | "two"            | 2         |
| "Sofisub"  | 1 (female) |                       | "three"          | 3         |
+------------+------------+-----------------------+------------------+-----------+

@FarmerMike252: Mike, please retry your test with this updated pywbemtools branch. The pywbem version does not matter (as long as it is supported by this version of pywbemtools, i.e. pywbem 0.17.0 or higher).

@andy-maier andy-maier changed the title Fixes #634: Added ValueMap value to instance table output (scalar and array elements) Fixes #634: Added value-mapped string to properties in instance table output Jun 27, 2020
@andy-maier andy-maier changed the title Fixes #634: Added value-mapped string to properties in instance table output [WIP] Fixes #634: Added value-mapped string to properties in instance table output Jun 27, 2020
@andy-maier andy-maier changed the title [WIP] Fixes #634: Added value-mapped string to properties in instance table output Fixes #634: Added value-mapped string to properties in instance table output Jun 27, 2020
@andy-maier andy-maier force-pushed the andy/valuemap branch 4 times, most recently from 6436c20 to 41fcf7e Compare June 28, 2020 18:52
Details:

* For integer-typed (scalar or array) properties that have a ValueMap qualifier,
  the output of instances in table format now includes the value of the Values
  qualifier in parenthesis, in addition to the integer value. (See issue #634)

  This change does not depend on the recent improvements of the ValueMapping
  support in pywbem versions 0.17.3 and 1.0.0b2 (See pywbem issue #2304), because
  the improvement only added support for passing a list or tuple of element values
  to ValueMapping.tovalues(), and because this change processes one value at
  a time, this new ability of the pywbem method is not used in pywbemcli.

* Added testcases for array properties.

* Added a TODO for an error that causes properties to be missed in instance
  table output. See issue #650.

Signed-off-by: Andreas Maier <andreas.r.maier@gmx.de>
@andy-maier
Copy link
Contributor Author

Merged it. Karl agrees that for now, we won't add options to enable or disable the behavior. If there are issues with this change, or requests for new functionality around it, please open a new issue.

@andy-maier andy-maier merged commit 40499be into master Jun 30, 2020
@andy-maier andy-maier deleted the andy/valuemap branch June 30, 2020 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants