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

Feature/get environment #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

atta
Copy link
Contributor

@atta atta commented Jun 7, 2019

added get_environment to napalm-onyx so you can make use of it in https://github.com/digitalocean/netbox

Copy link
Collaborator

@samerd samerd left a comment

Choose a reason for hiding this comment

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

Thanks @atta for your contributions.
really appreciate it.
please see my comments.
In addition, please remove your merge commit.
you should use rebase always to avoid merge commits.

show_temperature = json.loads(self.device.send_command('show temperature | json-print'))
show_resources = json.loads(self.device.send_command('show resources | json-print'))

if 'MGMT' in show_fan.keys():
Copy link
Collaborator

Choose a reason for hiding this comment

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

using key() is not efficient.
you can use:
if 'MGMT' in show_fan:

or:
for fan in show_fan.get('MGMT', dict()):

this comment for all resource types below

@wrap_test_cases
def test_get_environment(self, test_case):
"""Test get_facts method."""
modale_environment = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: should be: model_environment

"temperature": dict,
}
_environment = self.device.get_environment()
assert helpers.test_model(modale_environment, _environment)
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't you iterate over all dict items?

@atta atta force-pushed the feature/get_environment branch from b62d7f3 to 6b1d6d7 Compare June 11, 2019 07:51
@samerd
Copy link
Collaborator

samerd commented Jun 11, 2019

@atta ,
I still see the merge commit.
please remove it, or create a new PR without it

@FlorianHeigl
Copy link

FlorianHeigl commented Feb 9, 2021

since the driver isn't working anymore since many months it would be nice if you could review the acceptance policy.
the extra commit in the history is not pretty, but form over function is not a proven approach.
and the current state is MUCH less pretty!

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.

3 participants