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 to configure xinetd services from pillar #1

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

amontalban
Copy link

@amontalban amontalban commented Jul 5, 2017

This PR does the following:

  • Adds functionality to configure xinetd services using pillar data
  • Add configuration for tests with Kitchen CI and Travis CI

Andres Montalban and others added 7 commits July 5, 2017 15:21
- Adds functionality to configure xinetd services using pillar data
- Add configuration for tests with Kitchen CI and Travis CI
Allow to configure xinetd services from pillar
- Adds functionality to configure xinetd services using pillar data
- Add configuration for tests with Kitchen CI and Travis CI
This commit does the following:
As stated in https://docs.saltstack.com/en/latest/topics/releases/2019.2.0.html
Add the usage of `tojson` filter. This change is compatible with
2018.3.3 as per that docs.
Update kitchen, travis, Gemfile
Update for compatibility with SaltStack 2019.2
@aboe76 aboe76 requested a review from myii June 8, 2019 12:19
@myii
Copy link
Member

myii commented Jun 11, 2019

@amontalban Thanks for this PR and for your patience while waiting for a response! I've had a very brief look at this and it looks like a positive development. Since you first started this PR, there have been a number of changes and we've been rolling out a consistent pattern across all formulas, including the Kitchen and Travis configurations. We're also using "pre-salted" images to make our CI runs more efficient. A further step is the use of semantic-release to automate release tags, changelogs, etc.

My question to you: would you be willing to reshape this PR to fit what we're rolling out at the moment? I'm not asking you to do this yet -- I'm just starting the discussion, so that we can plan out the steps required, if you agree. Unless you have something very specific in your Kitchen/Travis configuration, it should be very simple to make the necessary modifications. For you to get an idea of what I'm talking about, please refer to the template-formula. If you are interested, I can also link you to all of the formulas that have been implementing this structure, since that is useful reference material.

Copy link
Member

@myii myii left a comment

Choose a reason for hiding this comment

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

Some feedback after an eyeball review, thanks.

@@ -15,9 +15,13 @@ Available states
.. contents::
:local:

``xinetd``
``xinetd.install``
----------
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
----------
------------------

----------

Installs the xinetd package and starts the daemon.


``xinetd.config``
----------
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
----------
-----------------

{%- from "xinetd/map.jinja" import xinetd with context -%}

{%- if xinetd.services is iterable %}
{%- for service, config in xinetd.services.iteritems() %}
Copy link
Member

Choose a reason for hiding this comment

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

iteritems is deprecated, please use items:

Suggested change
{%- for service, config in xinetd.services.iteritems() %}
{%- for service, config in xinetd.services.items() %}

- template: jinja
- context:
service: {{ service }}
config: {{ config|tojson }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
config: {{ config|tojson }}
config: {{ config|json }}

Unfortunately, we can't use |tojson just yet since we need to support 2017.7 for the time being. For more information about this, please refer back to saltstack-formulas/consul-formula#40 (comment) and the linked content.

Copy link
Member

Choose a reason for hiding this comment

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

There's also a second issue about using config here: saltstack-formulas/ufw-formula#7 (comment).

xinetd_{{ service }}_config:
file.managed:
- name: /etc/xinetd.d/{{ service }}
- source: salt://xinetd/files/config_template
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary for this PR but this is ripe for converting to TOFS (refer back to the template-formula for more info).

{% endif -%}
service {{ service }}
{
{% for k, v in config.iteritems() -%}
Copy link
Member

Choose a reason for hiding this comment

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

iteritems is deprecated, please use items:

Suggested change
{% for k, v in config.iteritems() -%}
{% for k, v in config.items() -%}

Felipe Zipitria and others added 2 commits March 5, 2020 17:54
Signed-off-by: Felipe Zipitria <[email protected]>
Change to items for py3 compat
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