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

[eos] fix banner when transport is ssh #2135

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

Conversation

derekdainys
Copy link

When setting transport="ssh" the heredoc commands cause an issue because they are not a string.
This allows for heredoc commands to not be converted when using "ssh" transport

@derekdainys derekdainys changed the title fix banner when transport is ssh [eos] fix banner when transport is ssh Jul 30, 2024
@ktbyers
Copy link
Contributor

ktbyers commented Aug 2, 2024

@bewing You know a lot more about this area of the code? Any objections?

@bewing
Copy link
Member

bewing commented Aug 6, 2024

Can we add a SSH test double class to https://github.com/napalm-automation/napalm/blob/develop/test/eos/conftest.py and possibly start testing some of the differences in behavior between the SSH and eAPI transports/drivers/devices?

@ktbyers
Copy link
Contributor

ktbyers commented Aug 6, 2024

@bewing Do we want to do this in conftest.py or have a separate test/eos and test/eos_ssh folder.

The latter is probably more consistent with what we did for other drivers (though we also have more code separation at the driver level between nxos and nxos_ssh (and between iosxr and iosxr_netconf).

Ultimately, I am fine with either/both (though probably have a preference for separate test folders for the different drivers+transport).

@bewing
Copy link
Member

bewing commented Aug 10, 2024

I also think eos_ssh folder makes more sense. I'll look at cribbing what's necessary from the existing _ssh folders and the standard EOS test double to arrange the boilerplate. @derekdainys if you can actually collect the data for the unit test it would be great - let me know on slack if you need any guidance

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