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 disabling services or just ignoring them. #453

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

cr1st1p
Copy link
Contributor

@cr1st1p cr1st1p commented Feb 17, 2020

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

Describe the changes you're proposing

It allows for minion, master, and salt-api to either ignore the state of the service or instead of enabling it, to disable it.

Pillar / config required to test the proposed changes

Check the defaults file, and play with the keys
(master|minion|api).service.(ignore,enabled).

The reason behind these flags:

  • current code always enforced the service to be enabled and running. I don't think this should be really enforced upon the user; you should allow user specify otherwise
  • I actually also need the formula to not do anything related to the services.
    Reason: installing salt-master and salt-api inside a docker image, and in there I do not have systemd running (and current code also wrongly tries to use 'upstart' in an ubuntu container...)

Debug log showing how the proposed changes work

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

salt/api.sls Outdated Show resolved Hide resolved
@dafyddj
Copy link
Contributor

dafyddj commented Feb 20, 2020

Hi, just a comment that commit messages in this project must follow a particular format.
See here.

@cr1st1p
Copy link
Contributor Author

cr1st1p commented Feb 20, 2020

@dafyddj any advice on how to proceed now? i.e. I already have the branch and N commits.

@dafyddj
Copy link
Contributor

dafyddj commented Feb 20, 2020

Are you familiar with Git's interactive rebase?
Amending the message of older or multiple commit messages

Alternatively a maintainer could do it for you prior to merge.

Instead of the default service.running + enabled, you can control
the actual state, via pillar.
And you can even say 'state = ignore' and no state will be generated
to control the service
@cr1st1p cr1st1p force-pushed the allow-disable-services branch from 1a567a0 to 29ffd68 Compare February 20, 2020 16:47
@cr1st1p
Copy link
Contributor Author

cr1st1p commented Feb 20, 2020

thank you @dafyddj for the help!

@myii myii merged commit 10e8778 into saltstack-formulas:master Mar 11, 2020
@myii
Copy link
Member

myii commented Mar 11, 2020

@cr1st1p Apologies for the delay, merging based on the approval from @aboe76 (thanks for that).

@saltstack-formulas-travis

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants