nitram509 / ntgrrc

ntgrrc (Netgear Remote Control) a command line (CLI) tool to manage Netgear GS3xx switch series.
MIT License
30 stars 6 forks source link

Feature: Allow ntgrrc to manipulate port settings #38

Closed davidk closed 1 year ago

davidk commented 1 year ago

As this is a large change, if approved, this commit will allow ntgrrc to modify the port specific settings seen on the front page of the admin control panel (port name, speed, ingress/egress limit, flow control).

This functionality also introduces a break in how ntgrrc was structured before, as there are now port settings that are not PoE. Where it conflicted, PoE specific settings were renamed to indicate it.

Sample usage:

status (similar to PoE status)

$ ./ntgrrc port status -a test
| Port ID | Port Name     | Speed | Ingress Limit | Egress Limit | Flow Control |
|---------|---------------|-------|---------------|--------------|--------------|
| 1       | Uplink to Net | Auto  | No Limit      | No Limit     | Off          |
| 2       |               | Auto  | No Limit      | No Limit     | On           |
| 3       |               | Auto  | No Limit      | No Limit     | On           |
| 4       |               | Auto  | No Limit      | No Limit     | On           |
| 5       |               | Auto  | No Limit      | No Limit     | On           |
| 6       |               | Auto  | No Limit      | No Limit     | On           |
| 7       |               | Auto  | No Limit      | No Limit     | On           |
| 8       |               | Auto  | No Limit      | No Limit     | On           |

set (also similar to PoE functionality)

$ ./ntgrrc port set -a test -p2 -n '2nd port'
| Port ID | Port Name | Speed | Ingress Limit | Egress Limit | Flow Control |
|---------|-----------|-------|---------------|--------------|--------------|
| 2       | 2nd port  | Auto  | No Limit      | No Limit     | On           |

$ ./ntgrrc port status -a test
| Port ID | Port Name     | Speed | Ingress Limit | Egress Limit | Flow Control |
|---------|---------------|-------|---------------|--------------|--------------|
| 1       | Uplink to Net | Auto  | No Limit      | No Limit     | Off          |
| 2       | 2nd port      | Auto  | No Limit      | No Limit     | On           |
| 3       |               | Auto  | No Limit      | No Limit     | On           |
| 4       |               | Auto  | No Limit      | No Limit     | On           |
| 5       |               | Auto  | No Limit      | No Limit     | On           |
| 6       |               | Auto  | No Limit      | No Limit     | On           |
| 7       |               | Auto  | No Limit      | No Limit     | On           |
| 8       |               | Auto  | No Limit      | No Limit     | On           |

resolves #37

codecov-commenter commented 1 year ago

Codecov Report

Merging #38 (063c3ea) into main (ece4a15) will decrease coverage by 3.07%. The diff coverage is 27.04%.

@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   42.22%   39.16%   -3.07%     
==========================================
  Files          14       16       +2     
  Lines         675      858     +183     
==========================================
+ Hits          285      336      +51     
- Misses        381      511     +130     
- Partials        9       11       +2     
Impacted Files Coverage Δ
main.go 0.00% <ø> (ø)
poe_cycle.go 0.00% <0.00%> (ø)
poe_set_port.go 30.05% <10.00%> (ø)
port_set.go 25.98% <25.98%> (ø)
port_status.go 32.14% <32.14%> (ø)
poe_settings.go 70.88% <50.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

davidk commented 1 year ago

Awesome work, David. I do suggest switching to 0.9.0 release with this

PS: I'm not getting it, what's the breaking nature? From user's perspective this is an addition, no? The code changes I see are related because of a renaming, which is finde, and makes things more clear/explicit.

It's 100% a rename to split out 'poe' functions from being confused with the new 'port' functionality. If you are ok with the new naming convention/split i'll keep it as-is :sweat_smile:

I've updated the help docs for the port section with the parameters it accepts; will push this as 0.9.0 in the changelog, thanks!

nitram509 commented 1 year ago

It's 100% a rename to split out 'poe' functions from being confused with the new 'port' functionality. If you are ok with the new naming convention/split i'll keep it as-is 😅

👍 Great job, thank you.