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

fix: support InstanceType in saphostctrl #3512

Merged
merged 0 commits into from
Sep 8, 2022
Merged

Conversation

xiangce
Copy link
Contributor

@xiangce xiangce commented Sep 2, 2022

Signed-off-by: Xiangce Liu [email protected]

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue? -> BZ-1969572
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

From BZ-1969572, the "InstanceType" should be used as the identifier for SAP function_instance

  • Added the InstanceType to the fixed filter of saphostctrl
  • Remove the unused "CreationClassName" from the fixed filter
  • Add a full_type function to get the full InstanceType
  • check "Solution Manager Diagnostic Agent" for function_instance
  • Convert the Parser SAPHostCtrlInstances to a list

@xiangce xiangce changed the title feat: support InstanceType in saphostctrl fix: support InstanceType in saphostctrl Sep 2, 2022
Copy link
Contributor

@bfahr bfahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I noticed some possible rule issues but I think those are related to test data and not structural changes.

@@ -142,11 +143,11 @@ def test_saphostctrl_docs():

def test_saphostctrl():
sap = SAPHostCtrlInstances(context_wrap(SAPHOSTCTRL_HOSTINSTANCES_GOOD))
assert len(sap) == 10
assert sap.data[-2]['SapVersionInfo'] == '749, patch 401, changelist 1806777'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiangce it might be good to leave this test in too for data since you added it as a property to maintain backward compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

And fixed the test data in rules per the parser.

Thanks for checking!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bfahr - The items you pointed out were fixed, I went ahead and merged this PR. For further issues, please let me know.

@xiangce xiangce merged commit b965215 into master Sep 8, 2022
@xiangce xiangce deleted the sap_function_instance branch September 8, 2022 03:46
xiangce added a commit that referenced this pull request Sep 8, 2022
* feat: support InstanceType in saphostctrl

Signed-off-by: Xiangce Liu <[email protected]>

* fix and test the full_type

Signed-off-by: Xiangce Liu <[email protected]>

* Use full_type only to check function_instances

Signed-off-by: Xiangce Liu <[email protected]>

- Add 'Diagnostic Agents'
* fix typos
* revert the test cases of .data
* revert the doc of attr 'instances'

Signed-off-by: Xiangce Liu <[email protected]>
(cherry picked from commit b965215)
xiangce added a commit that referenced this pull request Sep 6, 2024
* feat: support InstanceType in saphostctrl

Signed-off-by: Xiangce Liu <[email protected]>

* fix and test the full_type

Signed-off-by: Xiangce Liu <[email protected]>

* Use full_type only to check function_instances

Signed-off-by: Xiangce Liu <[email protected]>

- Add 'Diagnostic Agents'
* fix typos
* revert the test cases of .data
* revert the doc of attr 'instances'

Signed-off-by: Xiangce Liu <[email protected]>
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.

2 participants