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

Environment Support #25

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

N0odlez
Copy link

@N0odlez N0odlez commented Mar 13, 2024

Added Environment support

Description

Added support for Environment. Tested with 2930F and 2930M standalone and 2030M in a stack

Types of Changes

What types of changes does your code introduce? Keep the ones that apply:

  • New feature (non-breaking change which adds functionality)

@N0odlez N0odlez requested a review from gcotone as a code owner March 13, 2024 14:09
@N0odlez
Copy link
Author

N0odlez commented Mar 13, 2024

I'm not sure if anyone can authorise the changes?
@hupebln @gcotone @mirceaulinic @chris8838

Would be nice to get this added as i have a whole environment with 2930 switches.

@gcotone
Copy link
Collaborator

gcotone commented Mar 25, 2024

Hi @N0odlez,

Thanks for your contribution. Could you add unit tests for the new method? -> https://github.com/napalm-automation-community/napalm-arubaos-switch/tree/development/test

@N0odlez
Copy link
Author

N0odlez commented Mar 27, 2024

Hi @gcotone,

I've added unit tests and also squashed to one commit.

Copy link
Collaborator

@gcotone gcotone left a comment

Choose a reason for hiding this comment

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

Could you check the comments? Additionally, I changed some permissions because the checks of your PR didn't run, hopefully on your next push they'll do.
Thanks.

Comment on lines +33 to +44
member = switch.pop("member")
if member == "":
member = 0
else:
member = "Member " + member

out["cpu"][member] = {
"%usage": float(switch.pop("cpu"))
}

memtotal += int(switch.pop("memtotal").replace(',', ''))
memfree += int(switch.pop("memfree").replace(',', ''))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason for using pop() to fetch the values from ephemeral dicts? the more natural choice should be get()

Comment on lines -507 to +494
return super(ArubaOSS, self).get_users()
return get_users()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this coming from a different change? seems unrelated to the PR.

Comment on lines +43 to +44
memtotal += int(switch.pop("memtotal").replace(',', ''))
memfree += int(switch.pop("memfree").replace(',', ''))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the increment for the stack switches use-case ? If that's the case I think it's probably better to report back the memory of the device running the control-plane/primary-switch instead of the sum of the whole stack.

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