Fix/environ correction

[B1ue-W01f]

PR progress checklist (to be filled in by reviewers)

• Changes to documentation are appropriate (or tick if not required)
• Changes to tests are appropriate (or tick if not required)
• Reviews completed

---

What type of PR is this?

Primary type

• [build] Changes related to the build system
• [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
• [ci] Changes to the continuous integration configuration
• [feat] A new feature
• [fix] A bug fix
• [perf] A code change that improves performance
• [refactor] A code change that neither fixes a bug nor adds a feature
• [revert] A change used to revert a previous commit
• [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

• [docs] Documentation changes
• [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

#59

Describe the changes you're proposing

Theres a few commits and some of them undo changes made that I later realised weren't neccesary, apologies.
Primarily its regarding an update to the concat_environ used in the config.environ state.
Comments have been added across the default/pillar.example files to guide users to the differences in approach between archive and repo install methods. Comments have been added to the default and repo pillar files to highlight the different intent.
Tests added to ensure the environ args are being added to the required files for node_exporter and prometheus, along with pillar values being added to the repo pillar.

Pillar / config required to test the proposed changes

The salt/pillar/repo.sls is ultimately the only pillar file with pillar changes and includes those required to identify the correction.

Debug log showing how the proposed changes work

Tests added to the repo inspec check. Ive verified through the deployed prometheus server the log.level=debug is being passed to its command line flags.

Documentation checklist

• Updated the README (e.g. Available states).
• Updated pillar.example.

Testing checklist

• Included in Kitchen (i.e. under state_top).
• Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
• Updated the relevant test pillar.

Additional context

None.

[noelmcloughlin]

If null args is passed then ARGS="" .. is that valid configuration?

        environ:
          args: {}
          
          ```

[B1ue-W01f]

Debian issue appears to be due to the --collector.systemd config argument being unavailable in the older packages.

The formula handles either scenario so switching it out from the test pillar for --log.level=debug instead

[B1ue-W01f]

If null args is passed then ARGS="" .. is that valid configuration?

        environ:
          args: {}
          
          ```

Yes, that's the configuration that ships with the package.

[B1ue-W01f]

@noelmcloughlin ah its not the required entry for oracle linux! It appears to be ALERTMANAGER_OPTS="" format for oracle linux, hence the failing tests

[B1ue-W01f]

Same with contos. The previous formula passed obviously because it didn't actually change the arguments file, leaving the repo provided file but not allowing arguments to be used in the pillar.

Leaving two approaches then:
Add support for centos/oracle-linux - could do with guidance on that as it makes it a lot more complex.
Exclude centos/oracle from deploying the args file and note the lack of feature support.

Thoughts or any better approach?

[noelmcloughlin]

I guess adding support involves creating a custom pillar file in tests/salt/pillars (or equivalent), updating kitchen.yaml with specific for each custom args needed. It's probably not major work, just something needed here. I don't understand why particular distros are changing format of configuration files, complicating things.

…

On Fri, Jun 25, 2021 at 1:36 PM B1ue-W01f ***@***.***> wrote: Same with contos. The previous formula passed obviously because it didn't actually change the arguments file, leaving the repo provided file but not allowing arguments to be used in the pillar. Leaving two approaches then: Add support for centos/oracle-linux - could do with guidance on that as it makes it a lot more complex. Exclude centos/oracle from deploying the args file and note the lack of feature support. Thoughts or any better approach? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADFUUQSXCHU2P3IXXNTFFEDTURZ6JANCNFSM47JSWSSA> .

[B1ue-W01f]

I guess adding support involves creating a custom pillar file in tests/salt/pillars (or equivalent), updating kitchen.yaml with specific for each custom args needed. It's probably not major work, just something needed here. I don't understand why particular distros are changing format of configuration files, complicating things.
…
On Fri, Jun 25, 2021 at 1:36 PM B1ue-W01f @.***> wrote: Same with contos. The previous formula passed obviously because it didn't actually change the arguments file, leaving the repo provided file but not allowing arguments to be used in the pillar. Leaving two approaches then: Add support for centos/oracle-linux - could do with guidance on that as it makes it a lot more complex. Exclude centos/oracle from deploying the args file and note the lack of feature support. Thoughts or any better approach? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#60 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQSXCHU2P3IXXNTFFEDTURZ6JANCNFSM47JSWSSA .

Yeah, unfortunately, its a different name for each file though i.e. ALERTMANAGER_OPTS, PROMETHEUS_OPTS etc which is going to have to be passed into the file.

Going to need something like:

node_exporter:
  environ:
    environ_arg_name: NODE_EXPORTER_OPTS

for centos and:

node_exporter:
  environ:
    environ_arg_name: ARGS  

for default, I guess, per component.

Pretty annoying. Ill take a look later on tonight.

[B1ue-W01f]

@noelmcloughlin let me know if you want anything changed but that's the handling for contos and oraclelinux added. Also part fix for arch linux failing previously - enough to get exporters going and be close to a fix for the main server. IMO there's a few issues with the source package and can see part is identified as a bug here so likely best to leave it as is for now.

[B1ue-W01f]

commit appears to fully comply with the failing lint error so intending to leave as is.

[noelmcloughlin]

CI is failing:

⧗   input: fix: rework to implement environment variables handling
⚠   footer must have leading blank line [footer-leading-blank]

What is the intention with storage.sls as its new file?

[B1ue-W01f]

CI is failing:

⧗   input: fix: rework to implement environment variables handling
⚠   footer must have leading blank line [footer-leading-blank]

What is the intention with storage.sls as its new file?

I cant identify the actual reason the CI failed there, the footer had a preceding blank line?

The storage.sls is to ensure correct user control of the data directory from storage.tsdb.path ; I don't believe the formula does this previously / couldn't find it, and the arch linux build partially fails for that reason currently. For the most part, it doesn't affect anything but it would avoid failures if the parameter is specified but ownership was not otherwise assigned.

[noelmcloughlin]

The commit probably has some sequence of chars which is breaking commit lint: conventional-changelog/commitlint#896 Can you rebase and edit that commit, otherwise, it fails CI every time. Just simplify it if necessary

[B1ue-W01f]

@noelmcloughlin Hey, are you happy to accept? Or anything else I can do to improve?

[noelmcloughlin]

LGTM

[noelmcloughlin]

thanks @B1ue-W01f

[saltstack-formulas-travis]

🎉 This PR is included in version 5.5.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[myii]

@B1ue-W01f I've got a question for you: is there a specific reason why you added .bundle/ to the ignores list in the .yamllint file?

prometheus-formula/.yamllint

Line 15 in 2c572d0

.bundle/

This file is managed across ~100 formulas in our organisation so it will end up being reset when the next standardisation takes place -- unless there's a good reason for it, of course. I'm not sure how YAML files will end up under .bundle/ in any situation. I would be interested to hear otherwise. Thanks.

[B1ue-W01f]

@myii Quite possibly not a good reason, first time working with your template.

But without that yamlint was going down into the bundle directory, such as:

Which does appear to contain .yml files across a large number of bundles.

[myii]

@myii Quite possibly not a good reason, first time working with your template.

But without that yamlint was going down into the bundle directory, such as:

Which does appear to contain .yml files across a large number of bundles.

@B1ue-W01f That's an excellent justification for it, I'll add that to the list of ignores across the org. Personally, I use rbenv to manage my gems (i.e. I don't install them locally) -- but some will do that, as you have, and our .yamllint configuration needs to handle that.

Appreciate your time taken to explain that!