Skip to content

Conversation

@b4ldr
Copy link
Contributor

@b4ldr b4ldr commented Oct 31, 2019

A very naive attempt at adding apt-mark support

@adrienthebo i saw an offer to help with this functionality on #5216 and wondered if would be able to comment on this change also, Its not quite working but i feel its getting close. however If i am miles away please say. thanks

@b4ldr b4ldr requested review from a team October 31, 2019 14:48
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@ciprianbadescu
Copy link
Contributor

@b4ldr, the direction is good.

There is an on-going implementation of the hold property at #7773, will it cover your needs?

We should think if it makes sense to merge the two requests and offer a new property supporting both hold and manual values.

@b4ldr
Copy link
Contributor Author

b4ldr commented Nov 5, 2019

@ciprianbadescu thanks for the feedback.

I have taken a look at #7773 and im not sure it does make sense to merge them. From a purely technical point of view you are not able to mark a package using dpkg so we couldn't move this functionality to the dpkg provider.

From a conceptual point of view i don't think the mark attribute needs to be exposed to the user. Any package managed [to be installed in some way] by puppet should just receive apt-mark manual $package. I don't think it makes senses for a user to set the apt-mark value to anything else

Finally it is possible for a package to have apt-mark hold and apt-mark manual

@ciprianbadescu
Copy link
Contributor

@b4ldr, thank you for answer

I have taken a look at #7773 and im not sure it does make sense to merge them. From a purely technical point of view you are not able to mark a package using dpkg so we couldn't move this functionality to the dpkg provider.

Right. On dpkg provider :manual value shall not be supported

From a conceptual point of view i don't think the mark attribute needs to be exposed to the user. Any package managed [to be installed in some way] by puppet should just receive apt-mark manual $package. I don't think it makes senses for a user to set the apt-mark value to anything else

I think we should leave the options open and allow any user to keep the current functionality.

Finally it is possible for a package to have apt-mark hold and apt-mark manual

Yes, but hold will prevent all changes on a package, ignoring auto/manual setting.

I also added a question on https://puppetcommunity.slack.com/archives/C0W1X7ZAL to get more feedback on this.

@b4ldr
Copy link
Contributor Author

b4ldr commented Nov 5, 2019

So would the suggestion be to have some new property [possibly :mark] in the dpkg provider that supported [:hold, :unhold]. Then have the apt provider to extend that to also support [:manual, :auto] and have :mark => :manual by default?

@ciprianbadescu
Copy link
Contributor

So would the suggestion be to have some new property [possibly :mark] in the dpkg provider that supported [:hold, :unhold]. Then have the apt provider to extend that to also support [:manual, :auto] and have :mark => :manual by default?

Yes (while I have to do some more check for the default value).
The initial mark implementation for hold feature shall be implemented as solution for https://tickets.puppetlabs.com/browse/PUP-1537

@joshcooper
Copy link
Contributor

Rekicking Travis

@joshcooper joshcooper closed this Jan 17, 2020
@joshcooper joshcooper reopened this Jan 17, 2020
@b4ldr b4ldr requested a review from a team January 20, 2020 16:05
@b4ldr
Copy link
Contributor Author

b4ldr commented Jan 20, 2020

have rebased, seems this change is now dependent on #7815

@mihaibuzgau
Copy link
Contributor

@b4ldr we've merged #7815 can you please rebase.

@puppetcla
Copy link

CLA signed by all contributors.

@b4ldr
Copy link
Contributor Author

b4ldr commented Feb 13, 2020

I have rebased but its been a while since i looked at this. will need to go through it properly later to refresh my thinking

@ciprianbadescu
Copy link
Contributor

ciprianbadescu commented Apr 6, 2020

@b4ldr , looks good. Can you add some spec tests for changed code - for example to test instances you can inspire from: https://github.com/puppetlabs/puppet/blob/master/spec/unit/provider/package/dpkg_spec.rb#L24

Similar to what you did for instances method, you need to update query method that is used after update to check a specific package instance and get the mark state there also

@b4ldr
Copy link
Contributor Author

b4ldr commented Apr 7, 2020

@ciprianbadescu I have had a go at adding some rspec but i have to admit im getting pretty out of my ruby/rspec depth and although my tests are green I'm pretty sure they are not actually testing anything. Wonder if someone more knowledgable could provide pointers :)

@b4ldr b4ldr force-pushed the PUP-6631 branch 2 times, most recently from ab0c896 to 6832a48 Compare April 7, 2020 14:24
@b4ldr b4ldr force-pushed the PUP-6631 branch 2 times, most recently from 2588749 to e09985d Compare April 29, 2020 10:54
@gimmyxd gimmyxd requested a review from a team July 20, 2020 10:54
@puppetcla
Copy link

CLA signed by all contributors.

@ciprianbadescu ciprianbadescu requested a review from a team July 21, 2020 12:18
@gimmyxd gimmyxd merged commit 2be0923 into puppetlabs:master Jul 22, 2020
@bluser86
Copy link

bluser86 commented Jan 13, 2021

I just want to give a heads up that this breaks a usecase for us since updating to puppet version 6.19. We install a local deb file using the apt provider to make use of its dependency resolution (something dpkg does not do). Since apt does not make use of the source attribute of the package resource we set the package name to the path where the deb file is stored on disk:

package { '/path/to/package.deb': 
    ensure => installed,
}

This is valid because apt allows installing packages using this method. Since the apt-mark call has now been added as an internal step to the package resource every time we install this local deb file with the apt provider we get the following error:

Could not evaluate: Execution of '/usr/bin/apt-mark manual /path/to/package.deb' returned 100: E: Unable to locate package /path/to/package.deb

The reason for this is that apt-mark expects an installed package name as its argument which seems directly taken from the name attribute of the package resource, which in our case is the path to the deb file.

The package does get installed but the puppet run does end with an error now every time we run it. Is there something that can be done about this?

@b4ldr b4ldr deleted the PUP-6631 branch January 13, 2021 12:11
@GabrielNagy
Copy link
Contributor

Hi @bluser86, sorry about this. I created PUP-10854 to track this issue and we'll work on fixing it.

@ciprianbadescu
Copy link
Contributor

#8495 should allow apt provider to install local packages

@bluser86
Copy link

@GabrielNagy No need to apologize! Thank you and @ciprianbadescu so much for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants