Skip to content

(PUP-10597) Remove deprecated held #8267

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 27, 2020
Merged

(PUP-10597) Remove deprecated held #8267

merged 2 commits into from
Aug 27, 2020

Conversation

@Dorin-Pleava
Copy link
Contributor

@Dorin-Pleava Dorin-Pleava commented Aug 10, 2020

No description provided.

@puppetcla
Copy link

puppetcla commented Aug 10, 2020

CLA signed by all contributors.

@Dorin-Pleava Dorin-Pleava force-pushed the PUP-10597/Remove_deprecated_held_from_apt branch from 7f0f0b4 to 9adf282 Compare August 11, 2020 07:20
@Dorin-Pleava Dorin-Pleava requested a review from a team August 11, 2020 09:49
@Dorin-Pleava Dorin-Pleava marked this pull request as ready for review August 11, 2020 09:49
@Dorin-Pleava Dorin-Pleava requested review from a team August 11, 2020 09:49
@gimmyxd
Copy link
Contributor

gimmyxd commented Aug 11, 2020

@Dorin-Pleava we should entirely remove "held" from ensure values, as it was replaced by apt mark: #7802

@Dorin-Pleava Dorin-Pleava force-pushed the PUP-10597/Remove_deprecated_held_from_apt branch 2 times, most recently from 79b917c to 7bce804 Compare August 11, 2020 13:28
@gimmyxd
Copy link
Contributor

gimmyxd commented Aug 17, 2020

Jenkins please test this on debian8-64,debian9-64,debian10-64

@Dorin-Pleava
Copy link
Contributor Author

Dorin-Pleava commented Aug 17, 2020

Jenkins please test this on debian10-64a,debian8-64a,debian8-32a,debian9-64a,debian9-32a

@gimmyxd
Copy link
Contributor

gimmyxd commented Aug 17, 2020

Jenkins please test this on debian9-64,debian10-64

gimmyxd
gimmyxd reviewed Aug 17, 2020
View reviewed changes
acceptance/tests/provider/package/dpkg_hold_true_package_is_latest.rb Outdated
@@ -1,4 +1,4 @@
test_name "dpkg ensure held package is latest installed" do
test_name "dpkg ensure hold package is latest installed" do
confine :to, :platform => /debian-8-amd64/
Copy link
Contributor

@gimmyxd gimmyxd Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please update this to debian-9 as debian-8 was dropped in puppet-agent main

acceptance/tests/provider/package/dpkg_hold_true_should_preserve_version.rb Outdated
@@ -1,4 +1,4 @@
test_name "dpkg ensure held package should preserve version if package is already installed" do
test_name "dpkg ensure hold package should preserve version if package is already installed" do
confine :to, :platform => /debian-8-amd64/
Copy link
Contributor

@gimmyxd gimmyxd Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please update this to debian-9 as debian-8 was dropped in puppet-agent main

@Dorin-Pleava Dorin-Pleava force-pushed the PUP-10597/Remove_deprecated_held_from_apt branch from 7bce804 to ee97ee7 Compare August 18, 2020 06:43
ciprianbadescu
ciprianbadescu reviewed Aug 18, 2020
View reviewed changes
lib/puppet/provider/package/dpkg.rb
Comment on lines 176 to 183
def deprecated_hold
def hold
if package_not_installed?
self.install
end
hold
end

def hold
Copy link
Contributor

@ciprianbadescu ciprianbadescu Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we still need to call self.install in this step, as it may run the install twice

Copy link
Contributor Author

@Dorin-Pleava Dorin-Pleava Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it seems it installs the package without doing the self.install here.
I think package_not _installed? method can also be removed as I only found it being used here.

@GabrielNagy
Copy link
Contributor

GabrielNagy commented Aug 18, 2020

I believe this works okay, but while testing I found a behavior that's not idempotent (also happens without changes from this PR).

The following will always report changes on Debian 10:

package { 'mc':
    ensure => present,
    mark   => none,
}

/cc @ciprianbadescu

@Dorin-Pleava
(PUP-10597) Also removed package_not_installed?
5a55bbb 
As provider.hold does not install packages, method
package_not_installed? is no longer needed.

Also removed package_not_installed? tests and tests that expect
provider.hold to call provider.install
@ciprianbadescu
Copy link
Contributor

ciprianbadescu commented Aug 19, 2020

package { 'mc': ensure => present, mark => none, }

Should be solved with bellow change:

--- a/lib/puppet/provider/package/dpkg.rb
+++ b/lib/puppet/provider/package/dpkg.rb
@@ -74,7 +74,7 @@ Puppet::Type.type(:package).provide :dpkg, :parent => Puppet::Provider::Package
       elsif ['config-files', 'half-installed', 'unpacked', 'half-configured'].include?(hash[:status])
         hash[:ensure] = :absent
       end
-      hash[:mark] = :hold if hash[:desired] == 'hold'
+      hash[:mark] = hash[:desired] == 'hold' ? :hold : :none
     else
       Puppet.debug("Failed to match dpkg-query line #{line.inspect}")
     end

@ciprianbadescu ciprianbadescu requested a review from a team August 19, 2020 11:52
@puppetcla
Copy link

puppetcla commented Aug 21, 2020

CLA signed by all contributors.

@GabrielNagy GabrielNagy mentioned this pull request Aug 25, 2020
Merged
@GabrielNagy GabrielNagy force-pushed the PUP-10597/Remove_deprecated_held_from_apt branch from 64544ca to 5a55bbb Compare August 25, 2020 12:31
@GabrielNagy GabrielNagy merged commit 52fc636 into puppetlabs:main Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@gimmyxd gimmyxd gimmyxd left review comments

@GabrielNagy GabrielNagy GabrielNagy approved these changes

@ciprianbadescu ciprianbadescu ciprianbadescu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Dorin-Pleava @puppetcla @gimmyxd @GabrielNagy @ciprianbadescu @mihaibuzgau