refactor(sls): make states more readable by spliting into sub-components

baby-gnu commented 2 years ago

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
[x] [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

The current salt-formula is quite hard to read with lots of jinja conditionnals in the middle of states which is against the current standards.

This PR introduces a refactoring of the states into subcomponents:

master
TODO

and provides backward compatible state IDs which only notify user that their references must be changed.

Pillar / config required to test the proposed changes

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

baby-gnu commented 2 years ago

One point, the windows tests are failing because the salt.master was called for that platform:

before, a conditionnal made the rendered SLS empty
now, I used conditionnal to check for platform and using salt.master on windows platform results in False return codes for the states

logs of salt.master on default-windows-2022-latest-py3

``` ID: salt-windows-excluded-test.fail_without_changes Function: test.fail_without_changes Name: Verify that current platform is not Windows Result: False Comment: Platform Windows is not supported Started: 07:44:17.269399 Duration: 0.999 ms Changes: ---------- ID: salt-master-package-install-pkg.installed Function: pkg.installed Name: salt-master Result: False Comment: One or more requisite failed: salt.windows-excluded.salt-windows-excluded-test.fail_without_changes Started: 07:44:40.025083 Duration: 0.0 ms Changes: ---------- ID: salt-master-config-files-file.recurse Function: file.recurse Name: C:\salt\conf/master.d Result: False Comment: One or more requisite failed: salt.master.package.install.salt-master-package-install-pkg.installed, salt.windows-excluded.salt-windows-excluded-test.fail_without_changes Started: 07:44:40.025083 Duration: 0.0 ms Changes: Name: C:\salt\conf/master.d/_defaults.conf - Function: file.absent - Result: Clean Started: - 07:44:40.025083 Duration: 1.0 ms ---------- ID: salt-master-service-running Function: service.running Name: salt-master Result: False Comment: One or more requisite failed: salt.master.config.files.salt-master-config-files-file.recurse Started: 07:44:40.034083 Duration: 0.0 ms Changes: Name: salt-minion-py3 - Function: pkg.installed - Result: Clean Started: - 07:44:40.034083 Duration: 923.292 ms ```

I prefer to have the failing SLS instead of believing that everything was fine to assign an sls to the wrong platform, but if the community prefer the previous behaviour I'm ok to revert that part.

Regards.

daks commented 2 years ago

One point, the windows tests are failing because the salt.master was called for that platform:
* before, a conditionnal made the rendered SLS empty

* now, I used conditionnal to check for platform and using `salt.master` on windows platform results in `False` return codes for the states
logs of salt.master on default-windows-2022-latest-py3

I prefer to have the failing SLS instead of believing that everything was fine to assign an sls to the wrong platform, but if the community prefer the previous behaviour I'm ok to revert that part.

Regards.

If salt.master is not compatible with Windows, I think it should be clearly documented in the README for the meta-state and therefore make the state return nothing instead of returning an error.

(I didn't read the PR itself for now)

baby-gnu commented 2 years ago

If salt.master is not compatible with Windows, I think it should be clearly documented in the README for the meta-state and therefore make the state return nothing instead of returning an error.

Thanks @daks, that's exactly the kind of feedback I want.

So, as an example, you prefer the following execution output:

# salt '*' state.apply salt.master
PC-760282.example.net:
----------
          ID: salt-master-install-skip
    Function: test.show_notification
      Result: True
     Comment: No salt-master state for Windows
     Started: 11:11:14.046875
    Duration: 0.0 ms
     Changes:   

Summary for PC-760282.example.net
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   0.000 ms

over this one:

# salt '*' state.apply salt.master
PC-760282.example.net:
----------
          ID: salt-windows-excluded-test.fail_without_changes
    Function: test.fail_without_changes
        Name: Verify that current platform is not Windows
      Result: False
     Comment: Platform Windows is not supported
     Started: 11:15:17.453125
    Duration: 0.0 ms
     Changes:   

Summary for PC-760282.example.net
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1
Total run time:   0.000 ms
ERROR: Minions returned with non-zero exit code

daks commented 2 years ago

Yes, it seems a better solution. I don't use Windows but if I did I wouldn't want to have an error when everything is fine. And I consider that everything is fine because salt-master is just not supported on Windows. I think this can be a problem if you run automated highstates and just check for errors: this will pop up everytime as an error when it's not.

Of course, another solution would be to just run the compatible states.

baby-gnu commented 2 years ago

Ok, I consider an error to attribute an sls to an incompatible minion.

But having errors that states can't be run could be an issue, I'll look at our it render with state warning instead of simple notification.

daks commented 2 years ago

Ok, I consider an error to attribute an sls to an incompatible minion.

You're not wrong. As I said, for my use case, I would simply not run the meta-state on Windows.

Let see what other people think about that. I have no strong position on this.

vveliev-tc commented 1 year ago

I think the error is better than the success message. not everyone knows windows is not supported for master.

As a user, if I write salt-call state.sls salt.master - if it's successful I consider that everything is installed.

saltstack-formulas / salt-formula