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

feat: ✨Add jinja settings support for golden config plugin #527

Merged

Conversation

cablesquirrel
Copy link
Contributor

@cablesquirrel cablesquirrel commented Jul 17, 2023

@cablesquirrel cablesquirrel changed the title feat: ✨ jinja2 env settings feat: ✨Add jinja settings support for golden config plugin Jul 17, 2023
@itdependsnetworks itdependsnetworks changed the title feat: ✨Add jinja settings support for golden config plugin [draft] feat: ✨Add jinja settings support for golden config plugin Jul 17, 2023
@itdependsnetworks itdependsnetworks changed the title [draft] feat: ✨Add jinja settings support for golden config plugin [WIP] feat: ✨Add jinja settings support for golden config plugin Jul 17, 2023
@itdependsnetworks itdependsnetworks marked this pull request as draft July 17, 2023 17:16
@alextremblay
Copy link

Hi @itdependsnetworks, Any chance you guys can cut a new release of nornir-nautobot and move this PR out of draft status?

@itdependsnetworks
Copy link
Contributor

@jvanderaa would you be able to get the next release of nornir-nautobot at?

@jvanderaa
Copy link
Contributor

@itdependsnetworks I think we are set to put that out. There was one thing that I think we were collaborating on furhter.

@jvanderaa
Copy link
Contributor

@itdependsnetworks release 1.5 for nornir-nautobot is out.

@alextremblay
Copy link

I see that nornir-nautobot v2.5.0 came out with support for this feature, but nautobot-plugin-nornir, which depends on this feature, still depends on nornir-nautobot v2.3.0

I've opened issue nautobot/nautobot-app-nornir#101 to track this

@itdependsnetworks itdependsnetworks marked this pull request as ready for review August 26, 2023 22:34
@itdependsnetworks itdependsnetworks changed the base branch from develop to next August 28, 2023 18:16
@jeffkala
Copy link
Contributor

@cablesquirrel
Copy link
Contributor Author

At the request of @jeffkala, define the 'jinja_env_trim_blocks' and 'jinja_env_lstrip_blocks' options in the plugin default settings dict
@cablesquirrel
Copy link
Contributor Author

@jeffkala per your request, I changed how the defaults are defined. Please review my changes and let me know if that is correct.

| sot_agg_transposer | "mypkg.transposer" | None | A string representation of a function that can post-process the graphQL data. |
| per_feature_bar_width | 0.15 | 0.15 | The width of the table bar within the overview report |
| per_feature_width | 13 | 13 | The width in inches that the overview table can be. |
| per_feature_height | 4 | 4 | The height in inches that the overview table can be. |
| jinja_env_trim_blocks | True | True | A boolean to represent whether the jinja2 option for "trim_blocks" should be enabled for intended config rendering |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After getting internal feedback, I think the point @jeffkala and @electro47h is that there are many variables one could set for Jinja, I think the list is:

  • block_start_string
  • block_end_string
  • variable_start_string
  • variable_end_string
  • comment_start_string
  • comment_end_string
  • line_statement_prefix
  • line_comment_prefix
  • trim_blocks
  • lstrip_blocks
  • newline_sequence
  • keep_trailing_newline
  • extensions
  • optimized
  • undefined
  • finalize
  • autoescape
  • loader
  • cache_size
  • auto_reload
  • bytecode_cache
  • enable_async

It probably makes sense to support them, as a dictionary, with the 3 variables you have set as defaults. So, there should likely be one variable called jinja_env that is set. If you want to have a magic method of some sort such that anything set with jinja_env_* automagically gets into the dictionary, I could buy that as well, as long as it is in addition to the dictionary method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. None of those were possible to set before, but since they will be now, they should be included. This will take a good bit of rework on my end. I'm going to draft this PR and work on it later this week if time allows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for the masses here. For an option such as 'undefined', how would that look in the nautobot_config.py file for a setting?

from jinja2 import StrictUndefined

Environment(undefined=StrictUndefined, lstrip_blocks=...)

It appears most of the settings are boolean or string values, but a few of the options listed for Jinja are enumerations or classes. If the user intended to customize that setting in their nautobot_config.py file, would they have to add the import, or is there another way to represent that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user intended to customize that setting in their nautobot_config.py file, would they have to add the import

Yes, they would have to add that to the import within nautobot_config.py. We could work around it, but the complexity is not really worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I have committed what my interpretation of the request is.

I'm out of the office tomorrow and will be unable to test. We also have to get our environment upgraded from 1.5.24 to >=1.6.1 to install this version of the plugin. I will work to get this tested when I can.

Please let me know if this is the correct architecture that you're looking for, or if I need to make changes. Happy to keep plugging away at this.

@cablesquirrel cablesquirrel marked this pull request as draft August 28, 2023 20:48
erhansen-cox and others added 3 commits August 28, 2023 21:42
Per request from @itdependsnetworks, add support for specifying all options passed into the environment creation in a setting named 'jinja_env'
@cablesquirrel
Copy link
Contributor Author

cablesquirrel commented Aug 29, 2023

Corrected variable naming in the settings, plugin, and docs to match the Jinja Environment variable names since we are now using an exact match to the function parameters that are expected. Commit: f35ef16

@itdependsnetworks itdependsnetworks changed the title [WIP] feat: ✨Add jinja settings support for golden config plugin feat: ✨Add jinja settings support for golden config plugin Aug 29, 2023
@itdependsnetworks itdependsnetworks marked this pull request as ready for review August 29, 2023 16:15
@itdependsnetworks itdependsnetworks merged commit cdf7faf into nautobot:next Aug 29, 2023
16 checks passed
@cablesquirrel cablesquirrel deleted the feat-jinja2-env-settings branch August 31, 2023 15:44
jmpettit pushed a commit to jmpettit/nautobot-app-golden-config that referenced this pull request Jan 30, 2024
)

* feat: 🔧 Add jinja settings support for golden config plugin

---------

Co-authored-by: Eric M. Hansen <eric.hansen@cox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants