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

[SNMP] Add profile for IBM Lenovo servers #15377

Merged
merged 8 commits into from
Jul 26, 2023

Conversation

TCheruy
Copy link
Contributor

@TCheruy TCheruy commented Jul 25, 2023

What does this PR do?

Add profile for IBM Lenovo servers
Depends on #15371

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@TCheruy TCheruy requested review from a team as code owners July 25, 2023 16:11
@ghost ghost added the integration/snmp label Jul 25, 2023
@github-actions
Copy link

github-actions bot commented Jul 25, 2023

Test Results

       6 files      6 suites   25m 29s ⏱️
   345 tests 345 ✔️        0 💤 0
1 533 runs  525 ✔️ 1 008 💤 0

Results for commit b62acd9.

♻️ This comment has been updated with latest results.

Base automatically changed from ThibaudC/SNMP-profile-IBM-datapower-gateway to master July 26, 2023 08:28
name: diskTable
OID: 1.3.6.1.4.1.2.3.51.3.1.12.2
symbols:
- name: ibm.disk
Copy link
Member

Choose a reason for hiding this comment

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

ibm.disk seems very generic even with ibm prefix. It might conflicts with other ibm.

I think we should prefix everything ibm.imm. to:

  • avoid conflicts
  • can make it easier for user to search for metrics related to IMM (Integrated Management Module) related things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, changed prefix to ibm.imm

Copy link
Member

@AlexandreYang AlexandreYang left a comment

Choose a reason for hiding this comment

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

LGTM overall, one comment on the prefix.

Copy link
Contributor

@NouhaManai96 NouhaManai96 left a comment

Choose a reason for hiding this comment

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

Do you know if the IMM-MIB is specific for just Lenovo servers ? and in this case should the metrics be prefixed with lenovo/imm as well (for example ibm.lenovo/lenovo.server/imm.tempReading instead of ibm.tempReading ?

@@ -0,0 +1,156 @@
extends:
- _base.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this since ibm.yaml already extends _base.yaml

Suggested change
- _base.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed prefix to ibm.imm as suggested by @AlexandreYang

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #15377 (b62acd9) into master (49cde9f) will decrease coverage by 0.14%.
Report is 5 commits behind head on master.
The diff coverage is 15.90%.

Flag Coverage Δ
snmp 57.60% <15.90%> (-1.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@TCheruy TCheruy requested a review from AlexandreYang July 26, 2023 10:46
@TCheruy TCheruy merged commit cbe645a into master Jul 26, 2023
@TCheruy TCheruy deleted the ThibaudC/SNMP-profile-IBM-Lenovo-server branch July 26, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants