napalm-automation / napalm-base

Apache License 2.0
32 stars 48 forks source link

`load_template`: config format based on the template extension #173

Closed mirceaulinic closed 7 years ago

mirceaulinic commented 7 years ago

For the moment... Related to #118

mirceaulinic commented 7 years ago

This will allow introducing: https://github.com/napalm-automation/napalm-junos/issues/70

dbarrosop commented 7 years ago

Reading #118 I don't think we need this helper code. Let's just ditch completely cls._config_format and do something like (junos example):

format = filename.split('.')[-1]
if format not in c.SUPPORTED_CONFIG_FORMATS: # Which resolves to ["text", "set", "xml"]
   format = "text"
self.device.load_config(format=format)

It's simpler to reason about and it's more flexible as you can use different formats on the same session.

mirceaulinic commented 7 years ago

I agree - it's much better like that, just:

self.device.load_config(format=format)

the format arg is currently not accepted by load_merge_candidate or load_replace_candidate. Shall we add it, or do you mean defining a new public method called load_config?

mirceaulinic commented 7 years ago

@dbarrosop changed to handle the configuration format based on the filename. Also tackles the issue reported by @ktbyers in #143, point 2: now you can call specifying the template name as my_template.j2 or my_template.jinja without appending the .j2 extension.

dbarrosop commented 7 years ago

Sorry I wasn't clear, the load_config on my example was the pyez one. I didn't recall the exact name so I made it up. So in my version of the implementation, napalm-base doesn't require any change. The logic is entirely done on the drivers that have this sort of capability.

mirceaulinic commented 7 years ago

So in my version of the implementation, napalm-base doesn't require any change. The logic is entirely done on the drivers that have this sort of capability.

Oh I see now. But that would require rewriting the whole load_template per driver, isn't it?

dbarrosop commented 7 years ago

Oh I see now. But that would require rewriting the whole load_template per driver, isn't it?

Not really, load_config relies on the driver's implementation of load_merge_candidate so we will have to implement the code there to keep load_config generic.