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

Add eos_fn0039_config optional arg to toggle FN 0039 on config commands #1212

Merged
merged 3 commits into from
May 15, 2020
Merged

Add eos_fn0039_config optional arg to toggle FN 0039 on config commands #1212

merged 3 commits into from
May 15, 2020

Conversation

mirceaulinic
Copy link
Member

No description provided.

@mirceaulinic mirceaulinic added this to the 3.0.1 milestone May 13, 2020
@mirceaulinic mirceaulinic requested review from ktbyers and bewing May 13, 2020 11:48
@mirceaulinic mirceaulinic mentioned this pull request May 13, 2020
Merged
@coveralls
Copy link

coveralls commented May 13, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 0.0% when pulling f8d85e5 on fn-0039-opt-arg into 84c48dc on develop.

@mirceaulinic
Copy link
Member Author

Travis build: https://travis-ci.org/github/napalm-automation/napalm/builds/686533583

@ktbyers
Copy link
Contributor

ktbyers commented May 13, 2020

The argument here seems very obscure fn0039.

Is there a more meaningful argument we could use? I would probably call it something like convert_newstyle_config and then reference fn0039 in the docstring.

@mirceaulinic
Copy link
Member Author

No particular preference from me, just wanted to have it reference the reasoning of this change, I find "new style" very vague... if Arista will change again, we might end up with new-new-style, who knows 🤣
Happy to change if there are any strong opinions on this.

@ktbyers
Copy link
Contributor

ktbyers commented May 13, 2020

I don't care about it enough one way or another and it too easily can be bikeshedding (and as you said new_style is very vague).

We'll let other people chime in and now that I have seen eos_fn0039_config it is growing on me.

ktbyers
ktbyers reviewed May 14, 2020
View reviewed changes
napalm/eos/pyeapi_syntax_wrapper.py
new_commands = [cli_convert(commands, self.cli_version)]
else:
new_commands = [cli_convert(cmd, self.cli_version) for cmd in commands]
fn0039_transform = kwargs.pop("fn0039_transform", True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this default to False if fn0039_transform doesn't exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I thought I'd default this to True as we may want to disable this behaviour only for the config-related methods (the getters would always convert the commands under the hood)... so it was mostly a matter of whether we update every getter (everywhere run_commands is called) or just _load_config. Not sure if I'm explaining this well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that makes sense.

So internal to NAPALM for show commands, we would do this automatic conversion. But then for any configuration changes by end users, they would default to no conversion (with an option they could specify that would do the conversion).

Is that the correct understanding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correct, much better worded like this. 😄

@ktbyers
Copy link
Contributor

ktbyers commented May 14, 2020

I am fine with the argument name. I had one other question during the review and besides that I thought everything looked good.

@mirceaulinic mirceaulinic merged commit 1076311 into develop May 15, 2020
@mirceaulinic mirceaulinic deleted the fn-0039-opt-arg branch May 15, 2020 09:27
mirceaulinic added a commit that referenced this pull request May 15, 2020
@mirceaulinic @ExaneServerTeam
@spinoshi @dependabot-preview @ktbyers
Prepare release 3.0.1 (#1203)
456f736 
* Bugfix/ios get interfaces counters mgmt0 (#1201)

* IOS: fix get_interfaces_counters when interface = canonical

Fixes #1200

mgmt0 interface is not translated to it's canonical name
on the 'show interface' command.

Thix fix attempt to use the expected Canonical name first
and fallback to the interface name in case of a KeyError is
raised

* Add test

* Make black happy !

* Version 3.0.1 (#1202)

* Junos: close configuration in case configure_private is set to avoid configure session to stay forever. (#1199)

This is done in commit_config  and discard_config functions.

Co-authored-by: Mircea Ulinic <mirceaulinic@users.noreply.github.com>

* Bugfix/ios get optics input na (#1206)

* IOS: fix get_optics when input is N/A

Extend the current check to ouput value to the input value.
Input value could also be N/A

Fixes #1205

* Add test

* Bugfix/ios get optics support vss (#1208)

* IOS: support get_optics on VSS Setup

Introduce a new class function: _is_vss

If the current swith is a VSS setup, run
the following commands an parse the output::

    show interfaces transceiver switch 1
    show interfaces transceiver switch 2

* Add test

* Make black happy

* Bump pytest from 5.4.1 to 5.4.2 (#1204)

Bumps [pytest](https://github.com/pytest-dev/pytest) from 5.4.1 to 5.4.2.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/master/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@5.4.1...5.4.2)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Mircea Ulinic <mirceaulinic@users.noreply.github.com>

* Update README.md and setup.py so branch has some differences (#1209)

* Fix cmd_verify and null character on NX-OS (#1214)

* Bump ddt from 1.3.1 to 1.4.1 (#1219)

Bumps [ddt](https://github.com/datadriventests/ddt) from 1.3.1 to 1.4.1.
- [Release notes](https://github.com/datadriventests/ddt/releases)
- [Commits](datadriventests/ddt@1.3.1...1.4.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* Add eos_fn0039_config optional arg to toggle FN 0039 on config commands (#1212)

Co-authored-by: Kirk Byers <ktbyers@twb-tech.com>

Co-authored-by: Exane Server Team <frederic.brin@exane.com>
Co-authored-by: spinoshi <simone.spinelli@geant.org>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Kirk Byers <ktbyers@twb-tech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktbyers ktbyers ktbyers approved these changes

@bewing bewing Awaiting requested review from bewing

Assignees
No one assigned
Labels
bug eos optional arg
Projects
None yet
Milestone
3.0.1
Development

Successfully merging this pull request may close these issues.
3 participants
@mirceaulinic @coveralls @ktbyers