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

Flip installed and present in Function ensure_packages #1196

Merged
merged 1 commit into from
Aug 9, 2021
Merged

Flip installed and present in Function ensure_packages #1196

merged 1 commit into from
Aug 9, 2021

Conversation

cocker-cc
Copy link
Contributor

Flip installed and present in Function ensure_packages.
Puppet-Type package defaults to installed and not to present (compare here ).
This Misconception lies in the Code since the most early Days.

With flipping these two Values, Rspec-Tests do not have to be adapted, when migrating from package {} to ensure_packages().

Flip "installed" and "present" in Function "ensure_packages".
Puppet-Type "package" defaults to "installed" and not to "present" (compare https://github.com/puppetlabs/puppet/blob/main/lib/puppet/type/package.rb#L148).
This Misconception lies in the Code since the most early Days.

With flipping these two Values, Rspec-Tests do not have to be adapted, when migrating from "package {}" to "ensure_packages()".
@cocker-cc cocker-cc requested a review from a team as a code owner July 27, 2021 23:33
@puppet-community-rangefinder
Copy link

ensure_packages is a function

Breaking changes to this file MAY impact these 505 modules (near match):

This module is declared in 319 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@adrianiurca
Copy link
Contributor

LGTM

@adrianiurca adrianiurca merged commit 842992e into puppetlabs:main Aug 9, 2021
@bastelfreak
Copy link
Collaborator

Hi Puppet,
can you please rethink if this is a backwards incompatible change? This change breaks all unit tests in modules that verify ensure state: voxpupuli/puppet-openvpn#405

@ekohl ekohl changed the title Fix ensure_packages Flip installed and present in Function ensure_packages Aug 9, 2021
@cocker-cc
Copy link
Contributor Author

Hi Puppet,
can you please rethink if this is a backwards incompatible change? This change breaks all unit tests in modules that verify ensure state: voxpupuli/puppet-openvpn#405

This may be true, but as ensure_packages() did it wrong all the Time, the now breaking Tests relied on this wrong behaviour. At this Point the Cat bites its own Tail. I prefer to correct ensure_packages() and let the Tests break. All there is to apply in the breaking Tests, is to convert from present to installed.

Exactly this is my Point: When I have package { 'blubb': } (with default ensure => installed) and Test for contain_package('blubb').with_ensure('installed') and then I decide to migrate to ensure_packages('blubb') then my Test breaks, because of ensure_packages's wrong Default ensure => present.

@bastelfreak
Copy link
Collaborator

I don't mind having this merged, I just think it's backwards incompatible and the next stdlib release should be 8.0.0 and not 7.x.

@ekohl
Copy link
Collaborator

ekohl commented Aug 9, 2021

I don't mind having this merged, I just think it's backwards incompatible and the next stdlib release should be 8.0.0 and not 7.x.

I'm not entirely sure whether I agree. While functionally it's a change, I think it doesn't really warrant a major version bump because for end users it doesn't change anything.

You can argue that the testing method that was linked was checking internal behavior of ensure_packages() because there was no other way to test it.

Another thing that we should consider is that many (most?) Puppet modules use git checkouts so the version number is really irrelevant. Weighing this against the very painful costs for stdlib (so many modules need updating), I don't think we should.

That said, I have changed the PR title because it was really not covering the intent of this PR. That makes for an unusable changelog entry.

kenyon added a commit to kenyon/puppet-openssl that referenced this pull request Aug 24, 2021
$package_ensure default needs to be changed due to
puppetlabs/puppetlabs-stdlib#1196
kenyon added a commit to kenyon/puppet-openssl that referenced this pull request Aug 24, 2021
$package_ensure default needs to be changed due to
puppetlabs/puppetlabs-stdlib#1196

Also convert docs to Puppet Strings format.
penguinspiral added a commit to penguinspiral/puppet-control-repo that referenced this pull request Aug 26, 2021
As of version 8.0.0 the 'puppetlabs-stdlib' module updated the
`ensure_packages` function from 'present' to 'installed':
puppetlabs/puppetlabs-stdlib#1196

This commit addresses the single usage of `ensure_packages` function in
a dependent component module 'theforeman-dns'.

Verification (complete suite):

* `pdk test unit`

Result:

* Finished in 1 minute 3.18 seconds (files took 1.09 seconds to load)
685 examples, 0 failures
kenyon added a commit to kenyon/puppet-bind that referenced this pull request Oct 12, 2021
kenyon added a commit to kenyon/puppet-ca_cert that referenced this pull request Mar 10, 2022
This allows for better compatibility with puppetlabs-stdlib version 8
and use of ensure_packages('ca-certificates') (such as the
puppet-openssl does) due to
puppetlabs/puppetlabs-stdlib#1196. Otherwise you
can get a duplicate resource declaration error due to the mismatch in
the ensure value.
kenyon added a commit to kenyon/puppet-ca_cert that referenced this pull request Mar 10, 2022
This allows for better compatibility with puppetlabs-stdlib version 8
and use of ensure_packages('ca-certificates') (such as the
puppet-openssl does) due to
puppetlabs/puppetlabs-stdlib#1196. Otherwise you
can get a duplicate resource declaration error due to the mismatch in
the ensure value.
pcfens pushed a commit to voxpupuli/puppet-ca_cert that referenced this pull request Mar 11, 2022
This allows for better compatibility with puppetlabs-stdlib version 8
and use of ensure_packages('ca-certificates') (such as the
puppet-openssl does) due to
puppetlabs/puppetlabs-stdlib#1196. Otherwise you
can get a duplicate resource declaration error due to the mismatch in
the ensure value.
kenyon added a commit to kenyon/puppet-nfs-1 that referenced this pull request Mar 11, 2022
@alexjfisher
Copy link
Collaborator

I don't mind having this merged, I just think it's backwards incompatible and the next stdlib release should be 8.0.0 and not 7.x.

I'm not entirely sure whether I agree. While functionally it's a change, I think it doesn't really warrant a major version bump because for end users it doesn't change anything.

@ekohl It looks like it breaks all of my actual catalogs. I've just run a catalog-diff against a branch with stdlib 8.2.0 vs 5.2.0 and I've got ~1600 compilation failures. Duplicate declaration of package resources. :(

@alexjfisher
Copy link
Collaborator

The following compiled before, and now it doesn't.

package { 'tzdata':
  ensure => present,
}

ensure_packages(['tzdata'])
Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[tzdata] is already declared at (file: /home/afisher/test.pp, line: 1); cannot redeclare (file: /home/afisher/test.pp, line: 5) (file: /home/afisher/test.pp, line: 5, column: 1) on node host.example.com

rehanone added a commit to rehanone/puppet-autorestic that referenced this pull request Jul 17, 2022
rehanone added a commit to rehanone/puppet-dockctl that referenced this pull request Jul 17, 2022
…abs/puppetlabs-stdlib#1196)

- Updated `pdk` template version.
- Added unit test coverage for Debian 11.
- Removed test coverage for Ubuntu 14.04.
kenyon added a commit to kenyon/puppet-nginx that referenced this pull request Dec 1, 2022
kenyon added a commit to kenyon/puppet-nginx that referenced this pull request Dec 1, 2022
kenyon added a commit to kenyon/puppet-nginx that referenced this pull request Dec 1, 2022
alexjfisher added a commit to alexjfisher/puppetlabs-stdlib that referenced this pull request Mar 13, 2023
This unbreaks the breaking change made in puppetlabs#1196

Also refactored to create a separate dispatch method for the case when
`packages` is a `Hash`, and having that call the main `ensure_packages`
method. This simplifies the code by only ever calling `ensure_resource`
instead of calling `ensure_resources` for hashes.

Defaulting `default_attributes` to an empty hash instead of `nil` in the
method signatures further simplifies the code.

Fixes puppetlabs#1252
alexjfisher added a commit to alexjfisher/puppetlabs-stdlib that referenced this pull request Mar 13, 2023
This unbreaks the breaking change made in puppetlabs#1196

Also refactored to create a separate dispatch method for the case when
`packages` is a `Hash`, and having that call the main `ensure_packages`
method. This simplifies the code by only ever calling `ensure_resource`
instead of calling `ensure_resources` for hashes.

Defaulting `default_attributes` to an empty hash instead of `nil` in the
method signatures further simplifies the code.

Fixes puppetlabs#1252
alexjfisher added a commit to alexjfisher/puppetlabs-stdlib that referenced this pull request Apr 20, 2023
This unbreaks the breaking change made in puppetlabs#1196

Also refactored to create a separate dispatch method for the case when
`packages` is a `Hash`, and having that call the main `ensure_packages`
method. This simplifies the code by only ever calling `ensure_resource`
instead of calling `ensure_resources` for hashes.

Defaulting `default_attributes` to an empty hash instead of `nil` in the
method signatures further simplifies the code.

Fixes puppetlabs#1252
alexjfisher added a commit to alexjfisher/puppetlabs-stdlib that referenced this pull request Apr 21, 2023
This unbreaks the breaking change made in puppetlabs#1196

Also refactored to create a separate dispatch method for the case when
`packages` is a `Hash`, and having that call the main `ensure_packages`
method. This simplifies the code by only ever calling `ensure_resource`
instead of calling `ensure_resources` for hashes.

Defaulting `default_attributes` to an empty hash instead of `nil` in the
method signatures further simplifies the code.

Fixes puppetlabs#1252
b4ldr pushed a commit to b4ldr/puppetlabs-stdlib that referenced this pull request Jun 22, 2023
This unbreaks the breaking change made in puppetlabs#1196

Also refactored to create a separate dispatch method for the case when
`packages` is a `Hash`, and having that call the main `ensure_packages`
method. This simplifies the code by only ever calling `ensure_resource`
instead of calling `ensure_resources` for hashes.

Defaulting `default_attributes` to an empty hash instead of `nil` in the
method signatures further simplifies the code.

Fixes puppetlabs#1252
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 17, 2023
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 17, 2023
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 17, 2023
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 17, 2023
rrotter added a commit to mlibrary/nebula that referenced this pull request Sep 18, 2023
Flip present to installed in cifs_mount_spec due to
puppetlabs/puppetlabs-stdlib#1196
rrotter added a commit to mlibrary/nebula that referenced this pull request Sep 18, 2023
Flip present to installed in cifs_mount_spec due to
puppetlabs/puppetlabs-stdlib#1196
rrotter added a commit to mlibrary/nebula that referenced this pull request Sep 18, 2023
Flip present to installed in cifs_mount_spec due to
puppetlabs/puppetlabs-stdlib#1196
rrotter added a commit to mlibrary/nebula that referenced this pull request Sep 18, 2023
Flip present to installed in cifs_mount_spec due to:
puppetlabs/puppetlabs-stdlib#1196
rrotter added a commit to mlibrary/nebula that referenced this pull request Sep 19, 2023
Flip present to installed in cifs_mount_spec due to:
puppetlabs/puppetlabs-stdlib#1196
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 19, 2023
rrotter added a commit to mlibrary/nebula that referenced this pull request Sep 19, 2023
Flip present to installed in cifs_mount_spec due to:
puppetlabs/puppetlabs-stdlib#1196
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 20, 2023
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 21, 2023
kenyon added a commit to voxpupuli/puppet-nodejs that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants