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

fix(package): use correct version parameter with packages #61

Conversation

javierbertoli
Copy link
Member

@javierbertoli javierbertoli commented Nov 6, 2021

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

This should fix #54

Describe the changes you're proposing

When installing using packages (the default), node:pkg:version is used to pin the package. But there was a typo
that caused the issue described in #54. Fixed it.

Also, the formula uses

  • node:version (defaulting to 13.12.0) as the complete version of the package to install when using source or archive installs, but
  • node:pkg:version as the package to pin the version when installing using packages, but taken directly from pillars.
  • node:pkg:version to set the version in the upstream url for the repos, defaulting to only the major part of the
    version (node:pkg:version: '14' in defaults.yaml)

This results in errors when installing using use_upstream_repo: true, as:

  • the repo will have a version, but the default package will have another
  • the pinning will fail, as it's not a valid version

This PR fixes all the above, by:

  • Using node:pkg:version correctly when pinning
  • Use the major part of node:version to set the upstream's url when setting use_upstream_repo: true (ugly, but better than the current situation)
  • correctly installs from package when using default repos

As a side effect, to fix the tests, updated the version to current upstream's version (17.0.1)

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

@javierbertoli javierbertoli force-pushed the fix-version-usage-on-package-install branch 2 times, most recently from b6fe4a3 to c3bd2ce Compare November 6, 2021 23:20
@javierbertoli javierbertoli requested a review from a team as a code owner November 7, 2021 00:06
@javierbertoli javierbertoli force-pushed the fix-version-usage-on-package-install branch from 397fed8 to a5cdd41 Compare November 10, 2021 10:43
* fix(package): remove double colon preventing correct version usage

* feat(package): when `use_upstream_repo: true`, guess URL's version
  from `node:version`'s major value, to be consistent where the major
  version is obtained from. This also allows to pick a minor version
  from the repo (if available), using `node:pkg:version`

* feat(version): update to latest LTS node version (16.13.0)

BREAKING CHANGE: `node:pkg:version` needs to be specified as the
full version number (ie, 16.13.0) instead of just the major number
in order to be able to properly install using packages and pin the
desired version.
@javierbertoli javierbertoli force-pushed the fix-version-usage-on-package-install branch from a5cdd41 to 6ce8259 Compare November 10, 2021 13:26
@javierbertoli javierbertoli merged commit df25b2e into saltstack-formulas:master Nov 12, 2021
@javierbertoli javierbertoli deleted the fix-version-usage-on-package-install branch November 12, 2021 22:34
@javierbertoli
Copy link
Member Author

Performing "selfie-merge" for this PR, in line with community conventions:- https://github.com/saltstack/salt/blob/develop/doc/topics/development/conventions/formulas.rst#get-involved-creating-new-formulas

@saltstack-formulas-travis

🎉 This PR is included in version 2.0.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.

[FEATURE] Ability to set exact major version in pkg repository
2 participants