feat: Allow to exclude dotfiles on pillar data

[seven-beep]

Hello,

Following https://github.com/ben-grande/qusal/issues/74, here is a draft for allowing to opt-out the dotfiles states on a pillar basis.

After reflection, I do not think it is necessary to create and propagate a pillar.top and pillar.sls on this repository : the states are automatically applied with default values.

A few thinking about pillars :

• Too many pillar resolution has been known to cause performances penalities https://saidvandeklundert.net/2019-11-18-saltstack-pillar-data-and-map-files/ (however we have a good marge before hitting 100.000 lines!)
• Implementing theses new files will means to rework the setup scripts and the rpm specs.
• The placement of theses files need to be thinked about. Right now the file hierachy put all formulas into qusal/salt, however there is a strict distinction between file_roots and pillar_roots as both files inside are top files and sls files.

As you mentioned you worried about qusal states that may not work without the dotfiles ones, I will try to help to identify / fix them in the following days.

[ben-grande]

Hello,

Following ben-grande/qusal#74, here is a draft for allowing to opt-out the dotfiles states on a pillar basis.

After reflection, I do not think it is necessary to create and propagate a pillar.top and pillar.sls on this repository : the states are automatically applied with default values.

A few thinking about pillars :

• Too many pillar resolution has been known to cause performances penalities https://saidvandeklundert.net/2019-11-18-saltstack-pillar-data-and-map-files/ (however we have a good marge before hitting 100.000 lines!)

Yes, thanks for the heads up.

• Implementing theses new files will means to rework the setup scripts and the rpm specs.

I don't understand yet what is needed to be reworked on the setup scripts. On the RPM specs, I believe it is just regeneration of the specs.

• The placement of theses files need to be thinked about. Right now the file hierachy put all formulas into qusal/salt, however there is a strict distinction between file_roots and pillar_roots as both files inside are top files and sls files.

That is something that needs to be fixed in all formulas.

As you mentioned you worried about qusal states that may not work without the dotfiles ones, I will try to help to identify / fix them in the following days.

Thanks. Some formulas that install scripts to ~/.local/bin for example, such as cargo, ocaml, pip, which is not added by default to PATH by OSes.

[ben-grande]

Oh, and sorry, I did not add workflows to dotfiles, only to qusal, so no linting occurred.

[seven-beep]

ben-grande @.***> writes:

• Implementing theses new files will means to rework the setup scripts and
  the rpm specs.

I don't understand yet what is needed to be reworked on the setup scripts. On the RPM specs, I believe it is just regeneration of the specs.

I am not familiar with rpm packaging so ... I will just not speak about what I don't know.

For the setup scripts, it depends if you want to ship pillar data and enable it by default. If yes, you need to fix first the placement of pillar_roots and file_roots. Then the setup script should copy the appropriate files in target destination and then enable the $pillar.top(s).

I fixed some of the issues you mentioned but I am waiting for you to validate a few points.

[ben-grande]

I will probably ask you to squash the commits, but some of the commits messages might also be merge (manually).

Some tips on the commit message of https://github.com/ben-grande/dotfiles/pull/1/commits/26f0f84e7ac9caeb680c9e0a404b6ecbc8cf07a2

1,2c1
< Fix: Prevent error return code on void includes
< The include statement will return an error code 20 if no data is included.
---
> fix: Prevent error return code on void includes
4,7c3
< Their is three ways to fix this:
<    - boilerplate jinja2 around each includes
<    - include a dummy file with at least a function that do nothing
<    - a function that does nothing in the included files
---
> The include statement will return error code 20 if no data is included.
9,10c5
< I chose the third as I found that signaling to the user the effects of its
< pillar configuration is a good thing.
---
> There are three ways to fix this:
12,14c7,17
< I also refined the jinja2 around the copy-all include to take care of more
< specific setups where an user could have disabled all dotfiles but is enabling
< some specifically.
---
> - Boilerplate Jinja around each includes
> - Include a dummy file with at least a function that does nothing
> - Function that does nothing in the included files
>
> The third option was chosen as signaling to the user the effects of its
> pillar configuration explains the action instead of omitting it, which
> could cause confusion.
>
> Jinja around the copy-all include was refined to take care of more
> specific setups where an user could have disabled all dotfiles but is
> enabling some specifically.

Fixes:

• Change I did/chose/refined X to what was done
• Grammar
• Use of jinja2 replaced to Jinja, as I prefer to talk about the language instead of a specific version, it will always be the Jinja version that Salt uses. Same thing would be of Python in case we are dealing with a single Python version.
• Capitalized items in the beginning of the list as it is not a continuation.

Looking at the git messages, I have committed some of these errors also such as grammar and the use of I did X, so just something to keep in mind to future commits such as the message that will be used when squashed.

[seven-beep]

No, the logic here is someone may set qusal:dotfiles:all to false and individually activate, eg: qusal:dotfiles:dom0 to true, allowing then a whitelist or a blacklist.

This is admissibly a bit contrived and while I think about it, to make it works properly, I should also edit the jinja of the other states in the same spirit.

Do you think this behavior is desirable ?

[seven-beep]

I will probably ask you to squash the commits, but some of the commits messages might also be merge (manually).

Some tips on the commit message of 26f0f84

1,2c1
< Fix: Prevent error return code on void includes
< The include statement will return an error code 20 if no data is included.
---
> fix: Prevent error return code on void includes
4,7c3
< Their is three ways to fix this:
<    - boilerplate jinja2 around each includes
<    - include a dummy file with at least a function that do nothing
<    - a function that does nothing in the included files
---
> The include statement will return error code 20 if no data is included.
9,10c5
< I chose the third as I found that signaling to the user the effects of its
< pillar configuration is a good thing.
---
> There are three ways to fix this:
12,14c7,17
< I also refined the jinja2 around the copy-all include to take care of more
< specific setups where an user could have disabled all dotfiles but is enabling
< some specifically.
---
> - Boilerplate Jinja around each includes
> - Include a dummy file with at least a function that does nothing
> - Function that does nothing in the included files
>
> The third option was chosen as signaling to the user the effects of its
> pillar configuration explains the action instead of omitting it, which
> could cause confusion.
>
> Jinja around the copy-all include was refined to take care of more
> specific setups where an user could have disabled all dotfiles but is
> enabling some specifically.

Fixes:

* Change _I did/chose/refined X_ to _what was done_

* Grammar

* Use of _jinja2_ replaced to _Jinja_, as I prefer to talk about the language instead of a specific version, it will always be the Jinja version that Salt uses. Same thing would  be of Python in case we are dealing with a single Python version.

* Capitalized items in the beginning of the list as it is not a continuation.

Looking at the git messages, I have committed some of these errors also such as grammar and the use of I did X, so just something to keep in mind to future commits such as the message that will be used when squashed.

Thank you for your insights.

Is it ok for you that I simply rebase the commits into a single commit with proper message ?

I have committed some of these errors also such as grammar and [...]

P.S.: I do not see the commit that you are referring to ?

[ben-grande]

Is it ok for you that I simply rebase the commits into a single commit with proper message ?

Yes, exactly. Autosquash is an option of rebase. But just after review is finished, I like that you did separate commits, it is easier during review process so I can distinguish what I already have reviewed. Thanks!

P.S.: I do not see the commit that you are referring to ?

55f46f2793aac3db73137769dc8792e55972af55

I was skipping it locally but best to comment out as it is not being used.

The above is badly written. Example of what not to do (use of the first person I).

[seven-beep]

I included your recommendations for the commit message but also split the whitelist/blacklist part on its own commit.

There is also example pillar files as mentioned previously and small efforts on the README to be more comprehensible.

Notice that I am still testing this against Qusal formulas but I did not found much time theses last two weeks on this matter.

[ben-grande]

I included your recommendations for the commit message but also split the whitelist/blacklist part on its own commit.

Thanks.

There is also example pillar files as mentioned previously and small efforts on the README to be more comprehensible.

Thanks, with that, I believe, will be much clearer to users.

Notice that I am still testing this against Qusal formulas but I did not found much time theses last two weeks on this matter.

There is no problem in that, I am just happy it is being done, also because this is the first pillar implementation, I'm being cautions as the same method will influence other formulas and you also made some changes. Totally understandable. I haven't had the same time as before also.

[seven-beep]

I rebased your recommendations into the last commit.