Skip to content
This repository has been archived by the owner on Mar 7, 2018. It is now read-only.

get_snmp without acl #115

Closed
wants to merge 1 commit into from
Closed

Conversation

ogenstad
Copy link
Contributor

No description provided.

@mirceaulinic
Copy link
Member

Hi @ogenstad is this a fix for #113?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.587% when pulling 2fc5d16 on ogenstad:snmp into 3aee29e on napalm-automation:develop.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 11, 2017

Coverage Status

Coverage remained the same at 79.587% when pulling 2fc5d16 on ogenstad:snmp into 3aee29e on napalm-automation:develop.

@ogenstad
Copy link
Contributor Author

@mirceaulinic, yeah it was just to show a different solution.

@mirceaulinic mirceaulinic added this to the 0.5.2 milestone Jan 11, 2017
@bewing
Copy link
Member

bewing commented Jan 11, 2017

Why is the regexp trying to match group? EOS documentation states that group shouldn't show up in that command.

https://www.arista.com/en/um-eos-4172f/eos-section-38-4-snmp-commands#ww1154277

@ogenstad
Copy link
Contributor Author

Have no idea about group, I didn't touch that part.

@dbarrosop
Copy link
Member

I have no idea who wrote that and because of the split of the repos seems that some bits of ownership has been lost :(

@mirceaulinic was that you?

@mirceaulinic
Copy link
Member

Yes, I have probably missed that. However, I didn't notice anything bad so far.

@dbarrosop
Copy link
Member

No worries, just dead code. Can we just remove it then?

@mirceaulinic
Copy link
Member

Yes, I think @bewing's proposal in #114 will not use that textfsm anymore.

@dbarrosop
Copy link
Member

Right... should we close this PR then?

@mirceaulinic
Copy link
Member

Anyway that group part is ignored when the regex is evaluated.

I let you decide which is more maintainable on the long term, I am perfectly fine with both approaches. I know you don't like TextFSM so I think we can close this one indeed.

@mirceaulinic
Copy link
Member

I am going to close this one, in the favour of #114.

Thanks @ogenstad!

@ogenstad ogenstad deleted the snmp branch January 13, 2017 15:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants