fix(debian): allow version to be user-provided, don't force v13

Yoda-BZH commented 4 years ago

PR progress checklist (to be filled in by reviewers)

[x] Changes to documentation are appropriate (or tick if not required)
[x] 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
[x] [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`?

Yes. nodejs default version is set to 14, the LTS version.

Related issues and/or pull requests

Describe the changes you're proposing

Debian repository is forced on version 13.

Pillar / config required to test the proposed changes

https://github.com/saltstack-formulas/node-formula/blob/d8a8264ccaea147b65049b2cc9bd8473d1a74ebc/node/osfamilymap.yaml#L31

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

pull-assistant[bot] commented 4 years ago

Best reviewed: commit by commit

Optimal code review plan

fix(debian): allow version to be user-provided, don't force v13

Powered by Pull Assistant. Last update 30ec653 ... 30ec653. Read the comment docs.

Yoda-BZH commented 4 years ago

no news ?

myii commented 4 years ago

@Yoda-BZH Let's see if @noelmcloughlin can review this PR.

A few related issues I've noticed:

There are no entries for node:pkg:version in pillar.example or even any of the test pillars -- I believe this should be added to at least the pillar.example and the default.sls test pillar.
If this is to be considered a breaking change, then the commit message should reflect that as outlined here.

noelmcloughlin commented 3 years ago

LGTM but Semantic-Release is complaining about the commit message.

⧗   input: Set node:pkg:version to 14 in defaults
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty

This PR has correct format, use that: fix(debian): allow version to be user-provided, don't force v13

Yoda-BZH commented 3 years ago

I was waiting for an approval before rebasing.

Thank you

noelmcloughlin commented 3 years ago

CI is passing so merging. Thanks @Yoda-BZH

myii commented 3 years ago

@Yoda-BZH @noelmcloughlin Need a bit of help here because this PR has caused a regression in the CI for Debian-based platforms. Unfortunately, the packages-formula CI also uses this formula for Debian platforms, so those are failing as well now.

                 ID: node-package-repo-pkgrepo-managed
           Function: pkgrepo.managed
               Name: deb https://deb.nodesource.com/node_.x buster main
             Result: False
            Comment: Failed to configure repo 'deb https://deb.nodesource.com/node_.x buster main': E: The repository 'https://deb.nodesource.com/node_.x buster Release' does not have a Release file.
            Started: 09:57:49.961756
           Duration: 7236.536 ms
            Changes:   
       ----------
                 ID: node-package-install-pkg-installed
           Function: pkg.installed
               Name: nodejs
             Result: False
            Comment: An error was encountered while installing package(s): E: The repository 'https://deb.nodesource.com/node_.x buster Release' does not have a Release file.
            Started: 09:57:57.209571
           Duration: 2261.848 ms
            Changes:

The issue here is that this breaks our "no pillar" approach, where formulas should work with sensible defaults without needing to even configure a pillar. In this case, this forces a user to add a pillar entry for the version they need.

A better approach could be to construct the repo.name URL in the actual SLS file instead (putting all the pieces together). Then a user could still provide the pkg.version and have it automatically update the URL as well.

Do you have any suggestions on how we should proceed, if not my proposal above?

Yoda-BZH commented 3 years ago

Hello @myii,

Sorry for breaking the pipelines.

Can you check https://github.com/saltstack-formulas/node-formula/pull/59 ? I'm not sure if this is the right way to implement what you're proposing.

Thank you

saltstack-formulas-travis commented 3 years ago

:tada: This PR is included in version 1.1.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

saltstack-formulas / node-formula