search

napalm-automation / napalm-base

Apache License 2.0
33 stars 48 forks source link

load_template searchpaths, take 2 #294

Closed bewing closed 6 years ago

bewing commented 6 years ago

Build pathlists inside the helper function, pass them all to Jinja2.

Explicitly throw an error if the template_path specified is not found (Discussion needed. Silently ignore and append normal search paths instead?)

dbarrosop commented 6 years ago

Would you mind adding a test testing the MRO, please? You can probably build a subclass of the MockDriver which is bundled with napalm-base.

Thanks!

dbarrosop commented 6 years ago

How are you testing the MRO for the templates?

bewing commented 6 years ago

https://github.com/napalm-automation/napalm-base/pull/294/files#diff-8d00f135ea6d344c135ab86b55599e18R129

We're checking the search path assembled, and ensuring that both FakeNetworkDriver and NetworkDriver's path strings are present, which implies that they were built correctly from the MRO

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 72.494% when pulling d23459393d6c3b76146ab47bf17651d112c5bdfc on bewing:lt_walk into 42729d5129fdaaf02b45be614d2449425c91daed on napalm-automation:develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 72.494% when pulling 89599767f16d8278f50a383aeb576558d875ecc5 on bewing:lt_walk into 42729d5129fdaaf02b45be614d2449425c91daed on napalm-automation:develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 72.494% when pulling 89599767f16d8278f50a383aeb576558d875ecc5 on bewing:lt_walk into 42729d5129fdaaf02b45be614d2449425c91daed on napalm-automation:develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.4%) to 72.494% when pulling 89599767f16d8278f50a383aeb576558d875ecc5 on bewing:lt_walk into 42729d5129fdaaf02b45be614d2449425c91daed on napalm-automation:develop.

dbarrosop commented 6 years ago

We should probably break down this into smaller functions at some point, this method is... confusing. Anyway, I left a couple of minor comments. Feel free to discard the comprehension list suggestion, I just find them easier to read.

bewing commented 6 years ago

Thanks for the catch and the suggestion. Before merge, I wanted one more call for comments on https://github.com/bewing/napalm-base/blob/13ecc398e70630dff337df14c57b69a0bde680ef/napalm_base/helpers.py#L44-L50, as it is a functionality change (Before we silently ignored bad paths, and looked in the driver's template directory. We could refactor now to just blindly trust the input, and have it be the first path passed into the FileLoader

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.8%) to 72.892% when pulling 13ecc398e70630dff337df14c57b69a0bde680ef on bewing:lt_walk into 42729d5129fdaaf02b45be614d2449425c91daed on napalm-automation:develop.

dbarrosop commented 6 years ago

I don't like doing those sort of sanity checks, it pollutes the code and makes it harder to read and I am of the opinion that users should be responsible of using the library properly and sending a valid path if a valid path is what the function demands. However, your change at least throws an error instead of swallowing it so I am fine with the change.