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

Allow hosts to be specified for subnet as well #29

Merged
merged 3 commits into from
Jul 30, 2019
Merged

Allow hosts to be specified for subnet as well #29

merged 3 commits into from
Jul 30, 2019

Conversation

aaannz
Copy link
Contributor

@aaannz aaannz commented Jun 15, 2018

This allows host entries to be defined under subnet.

@0xf10e
Copy link
Contributor

0xf10e commented Jun 15, 2018

Oh, nice! 👍

This allows host entries to be defined under subnet.
@aaannz
Copy link
Contributor Author

aaannz commented Jul 26, 2019

Anything more I can do to be this accepted?

@myii myii requested a review from baby-gnu July 26, 2019 14:46
@myii
Copy link
Member

myii commented Jul 26, 2019

@aaannz Sorry for the long delay. @baby-gnu has offered to have a look at this on Monday.

@baby-gnu
Copy link
Contributor

The change is not trivial, I'm trying to figure out how to test that ;-)

Copy link
Contributor

@baby-gnu baby-gnu left a comment

Choose a reason for hiding this comment

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

I finally managed to understand the changes, you put the host template to a dedicated jinja file to include it at different places.

@baby-gnu
Copy link
Contributor

Note that I bugged on {{ intendation }}, it should be {{ indentation }}.

It can be fixed in another PR.

Regards.

@aaannz
Copy link
Contributor Author

aaannz commented Jul 30, 2019

Thank you for the review. I changed it based on your feedback, including change for intendation to indentation in last commit.

Copy link
Contributor

@baby-gnu baby-gnu left a comment

Choose a reason for hiding this comment

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

I'm ok with the new changes.

@baby-gnu
Copy link
Contributor

baby-gnu commented Jul 30, 2019

Thank you for the review. I changed it based on your feedback, including change for intendation to indentation in last commit.

Thanks a lot. I'm looking at Jinja2 documentation to find a better way of doing indentation of included files. We can't do {% include 'foo.jinja' with context | indent(width=2) %}

Let's get that merged for now, I'll do a dedicated PR for the indent optimization.

@baby-gnu baby-gnu merged commit 9fc487f into saltstack-formulas:master Jul 30, 2019
@myii
Copy link
Member

myii commented Jul 30, 2019

Thanks for your patience and fast response @aaannz.

@baby-gnu Thanks for taking the time to review. A common way I've found indent being done in our formulas can be seen from this search:

I.e. something like:

{%- macro do_something(width=4) %}
{%-   filter indent(width) %}
...
{%-   endfilter %}
{%- endmacro %}

@baby-gnu
Copy link
Contributor

A common way I've found indent being done in our formulas can be seen from this search:

Thanks @myii, I think I found a better way:

{%- filter indent(width=4) %}
{%- include "./sub-template.yml.j2" %}
{%- endfilter %}

I'm making tests to take care of the first line and how to manage spaces.

Regards.

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.

4 participants