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.

Sign up for GitHub

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

Jump to bottom

Updating the mibs file from vmware #8388

Merged
merged 1 commit into from
Mar 20, 2018
Merged

Updating the mibs file from vmware #8388

merged 1 commit into from
Mar 20, 2018

Conversation

mikeSimonson
Copy link
Contributor

@mikeSimonson mikeSimonson commented Mar 15, 2018 •
edited
Loading

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@laf
Copy link
Member

laf commented Mar 15, 2018

You've added a whole heap of new MIBs which are already in the mibs folder. Please remove those from the vmware folder and only have vmware specific mibs there.

laf
laf requested changes Mar 15, 2018
View reviewed changes
Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

Please see my last comment

@tracernz
Copy link
Contributor

tracernz commented Mar 15, 2018 •
edited
Loading

Can you put the source these were retrieved from etc. in the commit message please? It would help a bit for anyone trying to look at how they came to be in future, and is good hygiene in general when pulling in 3rd party sources.

@mikeSimonson
Copy link
Contributor Author

How do you spot those that are vmware specific ?
Those only come from the vmware archive.

@tracernz
Copy link
Contributor

How do you spot those that are vmware specific ?

VMWARE-*

Those only come from the vmware archive.

Yeah, they included all the generic MIBs their products use too.

@murrant
Copy link
Member

murrant commented Mar 20, 2018 •
edited
Loading

You can check this by running:
composer install (to install development dependencies
./vendor/bin/phpunit tests/MibTest.php --group mibs --filter=vmware to check for problems in the vmware mib directory.
./vendor/bin/phpunit tests/MibTest.php --group mibs will check all mibs and also checks for duplicates, but it will also give you a lot of noise because our mibs aren't clean right now :)

laf
laf requested changes Mar 20, 2018
View reviewed changes
Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

Close, just need to remove these files:

mibs/vmware/README
mibs/vmware/list-ids-diagnostics.txt
mibs/vmware/notifications.txt
mibs/vmware/vc-alarms-65.csv

And rename the other files to remove the .mib extension.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

laf
laf approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for doing this.

@laf laf merged commit 607a7f8 into librenms:master Mar 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laf laf laf approved these changes

Assignees
No one assigned
Labels
Blocker 🚫
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mikeSimonson @CLAassistant @laf @tracernz @murrant @scrutinizer-notifier