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

Adding an execution module providing helpers for the NAPALM formulas #48879

Merged
merged 5 commits into from Aug 5, 2018

Conversation

@mirceaulinic
Copy link
Member

@mirceaulinic mirceaulinic commented Aug 2, 2018

What does this PR do?

Some time ago I started preparing some formulas for network configuration management with respect to the OpenConfig standards. The repositories are created, part of the formulas are available, e.g, https://github.com/saltstack-formulas/napalm-ntp-formula

Generally, all these formulas (unfortunately) only generate a text blob that is eventually loaded on the device. Therefore the formulas will 90% be some Jinja template with a considerable size. To shrink this size, and make it more human readable, I thought about having a module, that transforms blocks such as:

{%- if subif_cfg.config.get('description') %}
  description {{ subif_cfg.config.description }};
{%- endif %}

Into a more simplified (when reading, at least):

{{ salt.napalm_formula.render_field(subif_cfg.config, 'description') }}

With the same rendering result:

description "Some description";

Which does all these repetitive checks such as above, which would appear tens of times in a template. Besides, it also add the field name.

This module is generic enough, and it will be used by all the future NAPALM Formulas, e.g., https://github.com/saltstack-formulas/napalm-bgp-formula, https://github.com/saltstack-formulas/napalm-interfaces-formula etc.

Together with this, adding some more little helpers for various manipulation of the OpenConfig-like Python objects.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Aug 2, 2018

@mirceaulinic
Copy link
Member Author

@mirceaulinic mirceaulinic commented Aug 2, 2018

Ooops! It should be fixed now, sorry!

@rallytime rallytime requested a review from Aug 2, 2018
try:
from salt.utils import traverse_dict_and_list
except ImportError:
from salt.utils.data import traverse_dict_and_list
Copy link
Collaborator

@terminalmage terminalmage Aug 2, 2018

Since this code is going into develop, there is no need to do this try/except, since the function lives in salt.utils.data now. Not only that, there are three problems with this approach:

  1. Importing a function into the module's global scope will add it to the __salt__ dunder (i.e. __salt__['napalm_formula.traverse_dict_and_list'])
  2. This will also add this function to the docs, which will be confusing.
  3. Since we have compatibility functions in salt.utils for all the funcs that were moved for 2018.3, the first import will succeed and this will result in deprecation warnings.

Instead, you should simply import salt.utils.data and invoke the function with its full name.

Copy link
Contributor

@gtmanfred gtmanfred Aug 2, 2018

When possible, it would be great to also use __utils__['data.traverse_dict_list']() because then it will allow people to include that data.py file in their salt://_utils/ directory if they are unable to upgrade to the newer version of salt.

Copy link
Collaborator

@terminalmage terminalmage Aug 2, 2018

Doing it @gtmanfred's way will also prevent you from needing to import the module at all.

Copy link
Member Author

@mirceaulinic mirceaulinic Aug 3, 2018

Thanks guys for reviewing this and your thoughts. As Daniel mentioned, I made these imports having backwards potability in mind; using __utils__ sounds like a better idea, thought it would require backporting loads of util modules.

Re 2: Indeed, I thought about that too, but it seems like Sphinx is smart enough not to pick these functions (no idea why) and they don't appear in the documentation (tested with both Salt and our internal documentation).

Anyway, I will change to use __utils__ as it seems a better tradeoff.

Copy link
Member Author

@mirceaulinic mirceaulinic Aug 3, 2018

Updated, though I just wanted to point this out: porting utils.data means it also requires porting utils.dictupdate, utils.stringutils, and utils.yaml (as they are imported there).

This is fine, though when I need, e.g., files.fopen, or args.clean_kwargs, I would also need to port loads of others such as utils.jid, utils.path, utils.platform, utils.data... and here's the catch: salt.utils.args needs salt.utils.data - see https://github.com/saltstack/salt/blob/develop/salt/utils/args.py#L18 (which obviously won't be available). So not only that one basically needs to port almost all the modules under utils/ but also they need to be patched (i.e., replace imports like the one from utils.args from import salt.utils.data to import _utils.data plus updating all the code to use the new import. This sounds like a nightmare to me.

Looking at this vs. 1 & 3 I'm not sure which is the worst...

Copy link
Member Author

@mirceaulinic mirceaulinic Aug 3, 2018

What about the following:

from salt.version import __version_info__
if __version_info__ < (2018, 3):
    from salt.utils import traverse_dict_and_list as _traverse_dict_and_list
else:
    from salt.utils.data import traverse_dict_and_list as _traverse_dict_and_list

Or why not, even simpler:

try:
    from salt.utils.data import traverse_dict_and_list as _traverse_dict_and_list
except ImportError:
    from salt.utils import traverse_dict_and_list as _traverse_dict_and_list

This way we can ensure that the function won't be exported (note that I flipped the import order to prevent deprecation warning thrown on >= Oxygen, i.e., it will firstly try to use from salt.utils.data).

Does this sound like a better alternative?

Copy link
Collaborator

@terminalmage terminalmage Aug 3, 2018

This does seem like a better alternative.

try:
    from salt.utils.data import traverse_dict_and_list as _traverse_dict_and_list
except ImportError:
    from salt.utils import traverse_dict_and_list as _traverse_dict_and_list

However, I'm not sure why this should be necessary at all in develop.

Copy link
Contributor

@gtmanfred gtmanfred Aug 3, 2018

Yes, the question just comes down to, "How friendly to we want to be for allowing people to backport these modules?"

Copy link
Member Author

@mirceaulinic mirceaulinic Aug 3, 2018

Added 3e9ccdb - please let me know if this is okay for the time being.

@gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Aug 3, 2018

@rallytime rallytime merged commit d51cbc6 into saltstack:develop Aug 5, 2018
5 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants