feat(package): use apt-pinning to pin specific package version

[hatifnatt]

Available only on Debian family OS-es

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

#314 #358

Describe the changes you're proposing

Add ability to pin version provided via pillar using apt-pinning. Benefits of version pinning:

• prevent unwanted package upgrades during system wide upgrades like apt-get update; apt-get upgrade
• allow package downgrade, simple version specification is not enough for downgrade because related packages won't be downgraded, i.e. if you try to downgrade from version 3000 to 2019.2.3 with apt-get install salt-minion=2019.2.* you will face error because apt will try to install version 3000 of salt-common

All changes applicable only to Debian based OS-es.

No test written but tested on my Debian Jessie - Buster machines successfully performed upgrade from v. 2018 to v. 2019, v. 3000 and downgrade from v. 3000 to v. 2019.

Note
Sometimes restart of salt-minion take a lot of time, about 2 minutes and formula execution ends with an error Minion did not return. [No response] checking service status with systemctl status salt-minion.service will retun

● salt-minion.service - The Salt Minion
   Loaded: loaded (/lib/systemd/system/salt-minion.service; enabled)
   Active: deactivating (stop-sigterm) since ...

but after some time minion became available again.

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

[myii]

@hatifnatt Thanks for this proposal. One thought crosses my mind: this feature is already provided by the apt-formula:

• https://github.com/saltstack-formulas/apt-formula#aptpreferences
• pillar.example

I don't think this should be duplicated here. Is there a particular use-case that you can see that makes this an exception?

[hatifnatt]

@myii I think formula must be self-sufficient to a certain level, and if this cause some functionality overlap that's fine. IMO version pinning is very basic function and use one more formula for this is overkill. If somebody using another formula for this - no problem, this new pinning function is not mandatory.
And finally if you are worried about duplicated functionality one more thing to think about - this formula is configuring repository why not use apt formula for that purpose? 😄

[myii]

I think formula must be self-sufficient to a certain level, and if this cause some functionality overlap that's fine. IMO version pinning is very basic function and use one more formula for this is overkill. If somebody using another formula for this - no problem, this new pinning function is not mandatory.

Sure, I understand your point of view. Let's get some other opinions about this. What do you think @javierbertoli @aboe76?

And finally if you are worried about duplicated functionality one more thing to think about - this formula is configuring repository why not use apt formula for that purpose? smile

There's a couple of differences here. The first is that pkgrepo isn't limited to one os_family, so there's no available formula for that! The second is that pinning packages is more specific functionality than configuring a repo. It has to be handled robustly, which needs a bit more care in the implementation. And there is a specific formula for it, in this case.

[hatifnatt]

Version pinning can be done on other OS too, but implementation is very different from OS to OS I simply can't implement it universally. I am using pinning as a tool to make downgrade possible, another way - list all packages with versions when performing installation, not only salt-master or salt-minion but salt-common too, but I think pinning is better approach because it have additional benefits.

[daks]

Hello, just for information we use the pillar salt:version to force a specific salt version installation, on master and minion, and on Debian servers. I don't know if this MR adds any functionality but for us it works well (ex : we apply the needed salt.{minion,master} state when we want to upgrade)

[myii]

Thanks for mentioning that, @daks. Would salt:version fit your needs @hatifnatt, or is there an additional feature in this PR that should be considered?

[noelmcloughlin]

salt: version could be enough as long as unattended-upgrade package is not installed and humans/scripts behave themselves. Otherwise pinning is the guarantee of version at package manager level.

[hatifnatt]

@myii of course I'm using salt:version pillar but it doesn't solve problem which this MR can solve. salt:version pillar allow you to install exact version and upgrade to more recent version, but it doesn't allow to perform downgrade for the reasons that I described above.

[myii]

@daks @noelmcloughlin Thanks for your suggestions but it comes down to whether you are OK with this functionality being duplicated here when it's available in the apt-formula. I definitely appreciate @hatifnatt's reasoning. However, I'd prefer not to maintain this functionality in both formulas but neither do I want to block this PR if others are OK with it. I suppose I'm suggesting that something like this would do the trick in top.sls:

base:
  '*':
    - apt.preferences
    - salt.minion

@hatifnatt Don't feel I'm being overly hard on you! I'm trying to avoid any unnecessary bloat, when there's already some trimming that's needed from this formula (such as the pillar-based method of managing config files).

[daks]

I think I already used it for downgrade of a minor salt-minion version (from 2019.2.2 to 2019.2.0) but I just tried and didn't acheive the same from 3000 to 2019.2, so maybe it's not usable.

Like @myii I don't like the idea of duplicating the pinning functionality between the two formulas. For your information, we also use the idea of combining apt-formula and salt-formula in one state and we call this state instead of the formula's one.

[hatifnatt]

Fortunately fork is still an option. Too many human resources already spent discussing this relatively simple change.
Personally I don't want to use another formula to pin a single package. And more important I want specify version in one place, run formula and get desired state for my system. For me it will be probably easier to write my own simple formula for salt installation than maintain another public formula in my environment.

[noelmcloughlin]

The correct approach in systems development is to combine components to achieve a greater task. I.e. a microservices approach. That is the feedback two reviewers gave you.

Below is example state I run - normal practice for achieving A, B, C and avoiding monolithic or WET design approaches. Fork away if you find collaboration too difficult, but silo approach is not collaborative is it?

include:
  {{ '- epel' if grains.os_family in ('RedHat',) else '' }}
      {%- if grains.os_family not in ('Suse',) %}
  - ceph.repo
      {%- endif %}
  - packages.pkgs
  - packages.pips
  - packages.archives
  - golang
  - timezone
  - resolver.ng
  - docker
  - memcached
  - mysql.apparmor
  - apache      ### manages port 80
  - nginx       ### manages port 8088 (daemon or container)

[myii]

Fortunately fork is still an option. Too many human resources already spent discussing this relatively simple change.

I understand your frustration, @hatifnatt. However, one problem we've got across this whole org is that features are implemented in formulas but then the original authors don't pay any attention and leave the maintenance burden on the very few of us who are actually making time to review issues and PRs here. And that number has dwindled over the years.

One remedy I'm suggesting is that we adopt the CODEOWNERS file in each repo:

• https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners

So as new features are added, contributors will be automatically flagged when their implementations are being adjusted. Hopefully, that will help run things better, so that we're not just hanging around saying: "Hmm, I'm not quite sure what the original author intended...".

So say we adopted that for this PR, and all future changes that reference this pinning functionality come back to you. Would you be willing to help that bit of the maintenance effort?

Personally I don't want to use another formula to pin a single package. And more important I want specify version in one place, run formula and get desired state for my system. For me it will be probably easier to write my own simple formula for salt installation than maintain another public formula in my environment.

Formula inter-dependencies are a tough topic. And I'm sure what you're saying there is what so many are doing as well. It's difficult to find the balance for these public formulas. Sometimes I feel that each one is doing too much and it would be a far better idea to split them up into more specific repos. So if we had a salt-formula that was specific to Debian, say, then this PR would become much less contentious. Any thoughts about that?

[hatifnatt]

@noelmcloughlin when exactly does flexibility end and over complication begins? I stated my thoughts in my first replies - some overlapping in functionality is acceptable. My proposal is nothing like manage nginx with salt-formula, only to improve handling for salt packages which this formula is supposed to manage.
My point - there is no need to spend 3 weeks discussing that simple proposal, owner or maintainer found it irrelevant / not useful - no problem reject and it's done.

@myii I don't know exactly how I'm supposed to support those changes in future they are pretty simple, no complicated logic. I don't mind to put some effort in the future if there will be something I can help with and I will have time and relevant knowledge.

OS specific formulas is not a bad idea, formulas became more simple and it's became possible to use OS specific "tricks", but on the other hand there will be a lot of similar (in functionality) code between such formulas, not DRY.

[javierbertoli]

On one hand I agree that, existing formulas that already allow to pin versions for the debian's family of distros) or in a more general way, for a few different OSes, adding this pinning here, for a single distro and just a single package (salt-master only, right?) does just not seem a clean approach.

But, OTOH, I agree that, even if some formulas offer the version attribute to 'pin' the version salt installs when it runs, a rolling distro (ie, debian stable) or even a new version of a package like salt-master might get upgraded when apps like unattended-upgrades run, because there's no 'apt pin' in place. And in that case, a system-wide pinning solution is handy.

For that last situation, which is the one @hatifnatt is trying to solve here, I think that adding a pin.sls state in the template-formula taking into consideration all those distros that allow pinning, and later of applying it org-wide to other formulas, might be a good idea.

My 2c.

[hatifnatt]

@javierbertoli not only salt-master but all salt* packages, and not only one distro but for one os_family Debian and Ubuntu are covered for sure, other Debian derivatives must be covered too. I agree, not cleanest approach but I can't implement for distros I'm not using.

[myii]

My point - there is no need to spend 3 weeks discussing that simple proposal, owner or maintainer found it irrelevant / not useful - no problem reject and it's done.

@hatifnatt It's no secret that the whole SaltStack Formulas organisation was drifting without direction for a while. That's why a number of us decided to do something about it and have put in significant efforts over the last year or so, trying to restructure how everything works -- I thank them all for their efforts. To get an idea of what I'm talking about, have a look at saltstack-formulas/template-formula#21. There's actually much more than this that has been tackled and the only way we resolve these plans is via. discussion.

Do you think we've spent 3 weeks discussing the actual implementation of this PR? I recognise and appreciate your contributions to various formulas @hatifnatt, and I'm not questioning the quality of what you provide. This isn't about the implementation, this is about using this opportunity to make meaningful changes to how this org operates. And via. this discussion, @javierbertoli has hinted an excellent idea of actually offering pinning across all formulas as a built-in option -- arriving at this point specifically because of your proposal here.

@myii I don't know exactly how I'm supposed to support those changes in future they are pretty simple, no complicated logic. I don't mind to put some effort in the future if there will be something I can help with and I will have time and relevant knowledge.

What appears simple and uncomplicated to you today, may appear otherwise to the maintainers of this formula a few years down the line ("why in the world did the maintainers allow pinning when that should have been handed off to the relevant formulas?!"). Besides, the whole CODEOWNERS idea is about the wider picture, where more complex features are added, authors also agree to help with the maintenance, rather than dump it on whoever's next.

Would you mind if I added you to inaugural CODEOWNERS file for this repo, in terms of maintaining salt/pin.sls?

OS specific formulas is not a bad idea, formulas became more simple and it's became possible to use OS specific "tricks", but on the other hand there will be a lot of similar (in functionality) code between such formulas, not DRY.

So since we're effectively forced to support multiple platforms per formula, we need to make things as easy as possible for (future) maintainers, by keeping the codebase as trimmed as possible, avoiding duplication and pointing users to other available formulas. This is a general point that no longer applies to this PR, because of what's been discussed above.

[myii]

Thanks for the patience, @hatifnatt. I added you to the CODEOWNERS file for salt/pin.sls.

[saltstack-formulas-travis]

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀