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

connection_options extra gathering in a merged way #759

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

viktorkertesz
Copy link

Rewrote get_connection_options logic in a way that defaults, group and host connection_options.extra parameters will be merged. Priority is defaults -> group1, group2,... -> host
So a host defined extra parameter would override defaults or groups if the key is the same. All distinct parameters will be merged together.

Benefits

We can better segment our connection_options extra parameters. We can define partial extras in defaults, groups and hosts.

Downside of the new logic

If a user relies on that host defined extra will not merge but replace the default/groups, then the user might get additional parameters which might be unexpected.

Example

defaults.yaml:

---
connection_options:
    netmiko:
        extras:
              secret: nornir

hosts.yaml:

host1:
    connection_options:
        netmiko:
            extras:
                  port: 222

Old behavior

This will result in that host1 will NOT have the secret parameter. Host file replace the extras.

New behavior

host1 will have set the port 222 and the secret set as well. Host file will merge the extras.

TODO

Although I completed tests with the new behavior, I did not do production testing. I'm curious if this all would be interesting to merge into the project. If there is a chance, then I'd work on:

  • more comprehensive tests with more realistic inventory
  • Complete docs on the behavior. (Currently there is no docs at all on this part of the inventory, not even for the original behavior)

@dbarrosop
Copy link
Contributor

I am afraid this change can’t be considered until we decide it’s time for a new major version (nornir 3.x.y). Semver rules, which we adhere to, prohibit making changes to behavior and/or the API without changing the major version.

In any case, before continuing with this PR I’d suggest you to open a github discussion and discuss it with the community. I personally prefer the current behavior but if there is a certain amount of people that would rather see the new behavior we could see if there is anything that can be done here.

@dbarrosop
Copy link
Contributor

dbarrosop commented Dec 26, 2021

I have a question regarding your implementation. Using your example, what happens if I do this?

nr.inventory.defaults.connection_options[“netmiko”].password = “defined-at-runtime”

print(nr.inventory.hosts[“host1”].connection_options[“netmiko”].password

will it return “defined-at-runtime”? Given your PR description I’d assume so but looking at the implementation I am not so sure. Could you clarify the expected behavior and confirm it behaves that way? Maybe adding a test to avoid regressions even.

No need to fix (if there is anything to fix) or add the rest until we discuss the feature though. Just adding the comment for future reference.

@viktorkertesz
Copy link
Author

nr.inventory.defaults.connection_options[“netmiko”].password = “defined-at-runtime”

At the moment I modified only the .get_connection_options method behavior. Therefore modifying defaults' or groups' connection_options attribute won't affect the host's. Looking at .get_connection method, it won't re-run .get_connection_options at the moment if it was already initialized. A further patch could re-run it, to get the most recent merge of all defaults' and groups' connection_options. That is ugly and I would rework the current solution similarly to .extended_data() method.
Thanks for raising this problem!

Let me create a discussion topic on this...

@ktbyers
Copy link
Collaborator

ktbyers commented Oct 3, 2023

@viktorkertesz Are you still actively working on this or should this be closed?

@viktorkertesz
Copy link
Author

@ktbyers Unfortunately my idea did not catch the community. I stopped working on this since then. But if you still think it would be a nice feature, I try to make it nice with runtime changes in mind.

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