Skip to content
This repository has been archived by the owner on Sep 17, 2019. It is now read-only.

load_template: config format based on the template extension #173

Closed

Conversation

mirceaulinic
Copy link
Member

@mirceaulinic mirceaulinic commented Dec 22, 2016

For the moment...
Related to #118

@mirceaulinic mirceaulinic added this to the 0.21.1 milestone Dec 22, 2016
@mirceaulinic mirceaulinic changed the title Ignore _config_format load_template: Ignore _config_format Dec 22, 2016
@mirceaulinic
Copy link
Member Author

This will allow introducing: napalm-automation/napalm-junos#70

@mirceaulinic mirceaulinic force-pushed the CFG-FMT-HACK branch 2 times, most recently from ca020c7 to 374a4d1 Compare December 22, 2016 07:30
@dbarrosop
Copy link
Member

dbarrosop commented Dec 22, 2016

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
Copy link
Member Author

mirceaulinic commented Dec 23, 2016

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
Copy link
Member Author

@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.

@mirceaulinic mirceaulinic changed the title load_template: Ignore _config_format load_template: config format based on the template extension Dec 23, 2016
@dbarrosop
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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.

@mirceaulinic mirceaulinic modified the milestones: DISCUSSION, 0.21.1 Dec 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants