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

Add support for checking if an apt::keyring is up-to-date with checksums #1199

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NeatNerdPrime
Copy link

#1196

Summary

Provide a detailed description of all the changes present in this pull request.

Additional Context

Add any additional context about the problem here.

  • Root cause and the steps to reproduce. (If applicable)
  • Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

@CLAassistant
Copy link

CLAassistant commented Sep 7, 2024

CLA assistant check
All committers have signed the CLA.

@smortex smortex changed the title issues/1196 Add support for checking if an apt::keyring is up-to-date with checksums Sep 8, 2024
Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

That looks good but we also need to adjust the apt::source defined type so that we can pass the checksum / checksum_value there too through the key parameter.

NeatNerdPrime added a commit to NeatNerdPrime/puppetlabs-apt that referenced this pull request Sep 9, 2024
In response to @smortex comment:
puppetlabs#1199 (review)

* Trickled up the checksum-related parameters for `apt::source`
* Modified test for source, all green.
@NeatNerdPrime
Copy link
Author

Hello again.

I added the requested changes, but whiel doing so i stumbled upon a perhaps uncomfortable situation.

Given the changes in the source_spec.rb test: https://github.com/puppetlabs/puppetlabs-apt/pull/1199/files#diff-abe1d0bffeeb2d6d33970b8d3272b76455f72cf2541de5f85678bd76817446e4

I realize that the parameter 'key' in apt::source is misleading, since it actually refers to the (deprecated) method of managing keys with the apt::key defined resource. But at the same time it is also used for apt::keyring? That just don't feel alright to me.

It begs the question: "Should we add a parameter 'keyring' to the defined resource, being mutually exclusive to the 'key' parameter, that calls upon apt::keyring and apt::key respectively?"

This could make things more clear from a user perspective and - unless Im mistaken - should be backwards compatible.

What do you people think?

puppetlabs#1196

* Adds checksum and checksum_value parameter to apt::keyring, this should
  address issue/1196 as commented here puppetlabs#1196 (comment)
* Includes tests, all green.
In response to @smortex comment:
puppetlabs#1199 (review)

* Trickled up the checksum-related parameters for `apt::source`
* Modified test for source, all green.
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.

4 participants