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

Remove "source" comments from Saltify configs #316

Merged
merged 2 commits into from
Jun 16, 2017

Conversation

cmclaughlin
Copy link

I use Salt environments to provide each of my team mates the ability to develop
and test their Salt changes. And I've found that when we run this formula from
our environments against our salt-master, comments in some files change. For us
this represents an unwanted and unplanned change. I understand the intention -
to identify whow or why the file changed, but I firmly believe that we should
be able to run highstsate with test=True and only see intended changes. Here's
an example:

        ID: salt-cloud-providers
  Function: file.recurse
      Name: /etc/salt/cloud.providers.d
    Result: None
   Comment: #### /etc/salt/cloud.providers.d/saltify.conf ####
            The file /etc/salt/cloud.providers.d/saltify.conf is set to be changed
   Started: 20:01:28.586441
  Duration: 75.185 ms
   Changes:
            ----------
            /etc/salt/cloud.providers.d/saltify.conf:
                ----------
                diff:
                    ---
                    +++
                    @@ -1,4 +1,4 @@
                    -# This file is managed by Salt via salt://salt/files/cloud.providers.d/saltify.conf?saltenv=myenv
                    +# This file is managed by Salt via salt://salt/files/cloud.providers.d/saltify.conf?saltenv=dev

                     saltify:
                       provider: saltify

I use Salt environments to provide each of my team mates the ability to develop
and test their Salt changes. And I've found that when we run this formula from
our environments against our salt-master, comments in some files change. For us
this represents an unwanted and unplanned change. I understand the intention -
to identify how or why the file changed, but I firmly believe that we should
be able to run highstsate with test=True and only see intended changes. Here's
an example:

            ID: salt-cloud-providers
      Function: file.recurse
          Name: /etc/salt/cloud.providers.d
        Result: None
       Comment: #### /etc/salt/cloud.providers.d/saltify.conf ####
                The file /etc/salt/cloud.providers.d/saltify.conf is set to be changed
       Started: 20:01:28.586441
      Duration: 75.185 ms
       Changes:
                ----------
                /etc/salt/cloud.providers.d/saltify.conf:
                    ----------
                    diff:
                        ---
                        +++
                        @@ -1,4 +1,4 @@
                        -# This file is managed by Salt via salt://salt/files/cloud.providers.d/saltify.conf?saltenv=myenv
                        +# This file is managed by Salt via salt://salt/files/cloud.providers.d/saltify.conf?saltenv=dev

                         saltify:
                           provider: saltify
@0xf10e
Copy link
Contributor

0xf10e commented Jun 7, 2017

I would suggest to introduce a pillar-based config option to omit those comments instead of removing a feature I consider very useful.

@cmclaughlin
Copy link
Author

Yeah, I understand that point of view. On the other hand, we don't seem to do that anywhere else:

$ grep "This file is managed by Salt" * -R
salt/files/cloud.maps.d/_saltify.conf:# This file is managed by Salt
salt/files/cloud.profiles.d/_saltify.conf:# This file is managed by Salt
salt/files/cloud.providers.d/_saltify.conf:# This file is managed by Salt
salt/files/master.d/engine.conf:# This file is managed by Salt! Do not edit by hand!
salt/files/master.d/reactor.conf:# This file is managed by Salt! Do not edit by hand!
salt/files/minion.d/beacons.conf:# This file is managed by Salt! Do not edit by hand!
salt/files/minion.d/engine.conf:# This file is managed by Salt! Do not edit by hand!
salt/files/minion.d/reactor.conf:# This file is managed by Salt! Do not edit by hand!
salt/files/roster.jinja:# This file is managed by Salt! Do not edit by hand!

cmclaughlin pushed a commit to ShopStyle/salt-formula that referenced this pull request Jun 7, 2017
Submitted this against the upstream:

saltstack-formulas#316

Doing it here on our older version with the restart hack
@cmclaughlin
Copy link
Author

@0xf10e want to merge this or let it sit?

@0xf10e
Copy link
Contributor

0xf10e commented Jun 15, 2017

I would say it's a feature other formulas are missing then.
But I thought it was just about the env and not the whole URL. I'm pretty sure I've seen salt://… URLs in configfiles. Did you try grep -R {{ source }} $PATH_TO_FORMULAS instead of the "This file is managed by Salt" one?

PS: I shouldn't check.those mails before breakfast…

@cmclaughlin
Copy link
Author

@0xf10e I'd like you to go ahead and merge this and if you want a "feature" to be added create a follow-up issue. From my standpoint, I'm fixing a bug by making the formula idempotent and making the code more consistent by following existing patterns. Your idea is good, but it's beyond the scope of this PR.

@0xf10e
Copy link
Contributor

0xf10e commented Jun 15, 2017

Oh, I'm honored you want your PR by me.
Though in terms of idempotence I'd consider
the environment as input.
Maybe we should ask someone else about
this…

@aboe76
Copy link
Member

aboe76 commented Jun 15, 2017

@0xf10e I have a rather extensive salt-formula setup so looking in the config files
most formulas use:

This file is managed by Salt! Do not edit by hand!

see:

  • postfix-formula
  • clamav-formula
  • fail2ban-formula
  • openssh-formula
  • apache-formula
  • uwsgi-formula
  • users-formula
  • nginx-formula
  • libvirt-formula

some have both

  • ntp-formula
  • salt-formula

I don't mind removing the {{ source}} because it makes it consistent with the rest of the formula
or make it a debug option.

{% if debug = true %} {{ source }} {% endif %}

@0xf10e
Copy link
Contributor

0xf10e commented Jun 16, 2017

OK then.

@0xf10e 0xf10e merged commit 739be95 into saltstack-formulas:master Jun 16, 2017
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.

3 participants