Open viktorkertesz opened 2 years ago
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.
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.
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...
@viktorkertesz Are you still actively working on this or should this be closed?
@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.
Rewrote
get_connection_options
logic in a way thatdefaults
,group
andhost
connection_options.extra
parameters will be merged. Priority isdefaults
->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:
hosts.yaml:
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: