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

Update routeros.py #138

Merged
merged 2 commits into from
Nov 8, 2020
Merged

Conversation

svliron
Copy link
Contributor

@svliron svliron commented Oct 27, 2020

Adjust the terminal detection line to support forward slashes in device hostnames (connections fail without clear reason otherwise)

SUMMARY

My RouterOS units have forward slashes in their system identities. The way the regex is structured prevents terminal detection from succeeding in this case, and devices will fail silently. The regex is still probably overly strict, but it fixed my problem.

ISSUE TYPE
  • Bugfix Pull Request

Adjust the terminal detection line to support forward slashes in device hostnames (connections fail without clear reason otherwise)
@felixfontein
Copy link
Collaborator

Thanks for this fix! This also needs a changelog fragment.

BTW, we started moving the routeros plugins and modules to a new repository yesterday: https://github.com/ansible-collections/community.routeros Since we plan to backport bugfixes to this repository anyway, I guess we can also merge this PR here and then 'foreport' it to the new repo. I hope that's OK for everyone.

CC @heuels

Copy link
Contributor

@heuels heuels left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@felixfontein
Copy link
Collaborator

@svliron once the changelog fragment is there (and looks good), I'll merge.

@igloo777
Copy link

@svliron are you sure that hostname syntax convention allows to use slashes in hostnames?

@felixfontein
Copy link
Collaborator

@igloo777 which "hostname syntax convention" do you mean? Could you please elaborate? RouterOS seems to accept hostnames which include slashes (otherwise we wouldn't have this issue/PR).

@felixfontein
Copy link
Collaborator

For the changelog fragments, what do you all think of:

bugfixes:
- "routeros terminal plugin - allow slashes in hostnames for terminal detection. Without this, slashes in hostnames will result in connection timeouts (https://github.com/ansible-collections/community.network/pull/138)."

@igloo777
Copy link

igloo777 commented Nov 6, 2020

@igloo777 which "hostname syntax convention" do you mean? Could you please elaborate? RouterOS seems to accept hostnames which include slashes (otherwise we wouldn't have this issue/PR).

https://tools.ietf.org/html/rfc952

RouterOS may accept any symbols in identity (I want emoji). But then RouterOS should have dedicated hostname option in config.

@igloo777
Copy link

igloo777 commented Nov 6, 2020

For the changelog fragments, what do you all think of:

bugfixes:
- "routeros terminal plugin - allow slashes in hostnames for terminal detection. Without this, slashes in hostnames will result in connection timeouts (https://github.com/ansible-collections/community.network/pull/138)."

IMHo, better to break an execution with "rfc952 breaking symbol symbol found in hostname, fix your hostname" message in err output.

@felixfontein
Copy link
Collaborator

Feel free to create a PR to implement that. But be warned, it's not trivial, since you have to modify code in another collection for that (https://github.com/ansible-collections/ansible.netcommon/blob/c2aa1d6981d9dd5d6e824fdfae2df7c5eef46f50/plugins/connection/network_cli.py#L996-L1050).

@felixfontein
Copy link
Collaborator

Once CI passes, I'll merge as this clearly improves the current situation. I'll also fore-port it to the community.routeros repo.

@felixfontein felixfontein merged commit 3f3e0d0 into ansible-collections:main Nov 8, 2020
patchback bot pushed a commit that referenced this pull request Nov 8, 2020
* Update routeros.py

Adjust the terminal detection line to support forward slashes in device hostnames (connections fail without clear reason otherwise)

* Add changelog fragment.

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 3f3e0d0)
@felixfontein
Copy link
Collaborator

@svliron thanks for the PR!
@heuels @igloo777 thanks for your reviews and comments!

felixfontein added a commit to felixfontein/community.routeros that referenced this pull request Nov 8, 2020
* Update routeros.py

Adjust the terminal detection line to support forward slashes in device hostnames (connections fail without clear reason otherwise)

* Add changelog fragment.

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 3f3e0d0)

Fore-ported from ansible-collections/community.network#138.
felixfontein pushed a commit that referenced this pull request Nov 8, 2020
* Update routeros.py

Adjust the terminal detection line to support forward slashes in device hostnames (connections fail without clear reason otherwise)

* Add changelog fragment.

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 3f3e0d0)

Co-authored-by: svliron <[email protected]>
heuels pushed a commit to ansible-collections/community.routeros that referenced this pull request Nov 9, 2020
* Update routeros.py

Adjust the terminal detection line to support forward slashes in device hostnames (connections fail without clear reason otherwise)

* Add changelog fragment.

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 3f3e0d0)

Fore-ported from ansible-collections/community.network#138.
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.

4 participants