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 deb822 APT sources #1167

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

Conversation

jps-help
Copy link

@jps-help jps-help commented Mar 7, 2024

Summary

Adds a new defined type, apt::source_deb822, and corresponding EPP template for managing deb822 APT sources.

Works alongside the existing apt::source type.

TO DO

  • Currently only supports specifying 'Signed-By' as a space-separated list of absolute file paths, but the deb822 spec also allows for writing the GPG ASCII armored key in-line with the .sources file.
  • Unlike the existing apt::source type, doesn't currently support generating associated apt::keyring resources. Need to be created separately.
  • Proper spec tests.

Additional Context

  • deb822 APT sources have been supported by APT since v1.1
  • Ubuntu 24.04 will ship with the Ubuntu repo configured in the newer deb822 style
  • This module doesn't currently have a method for managing deb822 .sources files

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

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

@kenyon kenyon mentioned this pull request Mar 12, 2024
3 tasks
@jps-help
Copy link
Author

Inline ASCII GPG keys are still not working, but not necessary for an initial implementation.

Same with generating apt::keyring resources from within the apt::source_deb822 resource. But I don't really think this is necessary at all since you can just create the keyring resource separately.

Spec tests are done.

I'm happy with the state of this PR. Happy to hear people's thoughts/suggestions.

Copy link

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Another area of improvement would be to use this in acceptance testing, otherwise 👍

@bastelfreak
Copy link
Collaborator

@jps-help thanks for the PR! Can you please rebase against main to get rid of the merge commit?

@jamesps-ebi
Copy link

@jps-help thanks for the PR! Can you please rebase against main to get rid of the merge commit?

The fork has been rebased. Should be good to go.

bastelfreak
bastelfreak previously approved these changes Mar 26, 2024
@jamesps-ebi
Copy link

Woops... I have accidentally reset my fork force pushed while testing another PR.

Luckily I still have my original deb822 branch. I'll fix and push again.

Copy link

@ptrsny ptrsny left a comment

Choose a reason for hiding this comment

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

What exactly is your reason to add an additional resource instead of extending the apt::source resource with an Enum parameter with the values "list" and "source" or "legacy" and "deb822"?

Otherwise thanks for your work!

@jps-help
Copy link
Author

What exactly is your reason to add an additional resource instead of extending the apt::source resource

I think the main reason is just because of how different the two styles are. If you look at the current apt::source, many of the parameters take a single value, whereas, with deb822 parameters like suites and components can accept an array of values.
Sure we could just have one .sources file per suite or component of a repo, like with the current method, but I think it defeats the purpose of the newer deb822 style.

There might be a way we can extend the current type, I'm open to suggestions. My main focus was simply to add the new functionality without breaking the existing type at all.

@rrotter
Copy link

rrotter commented Jun 25, 2024

I'd suggest keeping this under apt::source for several reasons:

  • apt::source_deb822 would be a good name if this was an experimental interface, but deb822 is not "another way" it's the "new way", and (I think) will eventually be the only way. It should be presented as the default rather than an alternative, and especially not an alternative with a longer snake_cased_name.
  • Having two resource types makes deleting sources worse. Imagine needing code like:
apt::source_deb822 { 'testing123': ensure => 'absent' }
apt::source { 'testing123': ensure => 'absent' }
  • complicates migration: ideally upgrading to deb822 would be a one-line change to an existing apt::source; like adding 'type' => 'deb822' or better still a zero-line change like omitting 'type' => 'legacy'*
    *this latter suggestion would probably need to wait for a major release since it's a breaking change)

There might be a way we can extend the current type, I'm open to suggestions.

Indeed, I'd suggest adding an Enum['legacy','deb822'] that probably defaults to 'legacy' for now. I like the idea of using the modern names for the fields (suites vs release, etc), but we could have both, and have one be an alias for the other. Could accept either an array or a string in the new name and continue accepting only a string in the old field name. That way there's the option of taking advantage of everything new that deb822 offers, but also the option of just switching over with at little change as possible.

edit: also, using any of the new field names should implicitly force deb822, and trying to combine the new fields with the legacy source would be an error. See the handling of keyrings in the apt::source: key field for an example.

@jps-help
Copy link
Author

@rrotter All very good points. Let me take this back to the drawing board and try to implement with all these suggestions.

I think we will need a lot of new logic to handle the switch between the two versions, but in the end hopefully will be a better experience!

@jps-help
Copy link
Author

jps-help commented Jun 26, 2024

Okay... I have merged some new changes that integrate the deb822 functionality into the existing apt::source type.

This is very much a work-in-progress and has several points to consider:

  • I made the decision to add new parameter names for deb822 resources so they match with the content of the actual .sources file. This allows us to use different data types for those where appropriate.
  • We might be able to allow an easy transition if we can find a way to have the new parameters use the old parameters as default values. I played with this but didn't implement because I found a few issues with that.
  • Maintainers need to decide on the appropriate way to deprecate the existing method. Currently this change leaves the default as the current method. I tested a resource created using the current forge version, then upgrading to this version and saw no changes to the resource.
  • I added a switch source_format which controls this behaviour. The default is 'legacy', and the new format is 'deb822' but maybe this should be 'list' and 'sources'? Or are we assuming there won't be any major change to apt sources for a long time?

Comments are greatly appreciated.

TLDR; I used a case statement to separate the functionality and added a new Enum parameter to control it.

Also I left the separate manifest for apt::source_deb822 but can clean that up later.

@smoeding
Copy link

smoeding commented Jul 6, 2024

  • I added a switch source_format which controls this behaviour. The default is 'legacy', and the new format is 'deb822' but maybe this should be 'list' and 'sources'? Or are we assuming there won't be any major change to apt sources for a long time?

IMHO the 'list' and 'sources' options would be better since these are the file extensions that are eventually used. This makes it easier to remember the possible values.

In case that there will be another file format change in the future, the name of that format can easily be added here as allowed value. Otherwise 'deb822' would be the legacy format then.

@jps-help
Copy link
Author

Finally had a chance to update this. I've used "list" and "sources" as the possible values for source_format and also re-used that parameter as the file_suffix later in the code.

I've also removed the separate apt::source_deb822 type and some more failure conditions for deb822 style sources.

I think there will be more logic to add but can't think of anything right now. The only thing I see left are spec tests and README documentation but won't do those until we're happy with functionality.

@ptrsny
Copy link

ptrsny commented Jul 25, 2024

Thank you for implementing the approach of only having one source resource.

As far as I can see the only thing I am missing in your implementation is @rrotter s suggestion of making the migration a one line change. This means, that the values components <=> repos, suites <=> release, repo_trusted <=> allow_unsigned and uris <=> location are (in my understanding) nearly interchangeable. So in other words if the parameter repos is set, and source_format is set to source I expect that it first tries to use the components parameter, but as long as that is not set it falls back to use repos (and that for all parameter combinations listed above).

I understand the idea of having the parameter names mapped to the corresponding keys of the format that is used. The concern I have with this is that the definition will explode every time a new format is added (hopefully not). Saying this I suggest that the doubled parameters should stay the old ones OR the new ones, but not both and "only" adapt the parameter types and improve the documentation accordingly.

Thanks again for your work on this. (And sorry for the late comment)

@jps-help
Copy link
Author

making the migration a one line change.

Sounds like a good idea. Thinking on this again, it doesn't make sense to add what are essentially duplicate parameters.

I think it will require a lot more logic though since we need to allow the original String value and Array value where appropriate. But it should all be possible.

@jps-help
Copy link
Author

Migration between .list and .sources should now be much easier.

I have consolidated the parameters used for both formats so they now mostly share the same parameters.
There are a few of exceptions:

  1. $enabled - This is used exclusively by deb822 sources.
  2. $types vs $include - Both are used to specify which types to include ('deb' and/or 'deb-src'). The newer $types param has a default value and I found that $include being a hash was needlessly complicated for what is essentially a Boolean choice.
  3. $pin and $key - I've tried to implement these for deb822 sources but found it is quite complex. This can be revisited later but currently I just raise a warning that these are ignored for deb822 sources. We can always add apt::pin and apt::key resources separately anyway.

@jps-help
Copy link
Author

Okay. All suggestions have been implemented where possible.

I have now added spec tests, updated README and rebased against MAIN since we were quite a bit out-of-sync.

Unless there are any more notes, I think this is ready.

@jps-help
Copy link
Author

I saw main had some new commits, so I have rebased again.

@kenyon kenyon mentioned this pull request Nov 21, 2024
3 tasks
jamesps-ebi and others added 26 commits November 25, 2024 14:27
Should be roughly drop-in alternative to the existing apt::source type
Does not currently support inline ascii gpg key
Use 'sources' instead of 'source' as the setting_type parsed to
apt::setting

Fix the data type of apt::source::signed_by
Correctly handle newline/whitespace trimming for `signed_by` parameter.
Match the possible values to the file suffix of the created source
files.
Allow array values for certain parameters to allow easy switching between .list and .sources formats.
Convert string values to arrays where possible and warn the user.
Correctly compare data type when generating deb822 sources
Remove unused class parameters and descriptions
Remove references to unused deb822 parameters
Update parameter descriptions
Update deb822 example
Update warnings for $pin and $key usage with deb822. Currently
unsupported
Don't fail if $location is missing unless $ensure is 'present'
eliminate params.pp and create_resources()

params.pp and create_resources() are obsolete.

This module was converted to non-params.pp style puppetlabs#667, but was
reverted in puppetlabs#680. Using Hiera in modules and no params.pp are the
preferred styles these days.
Add support for deb822 .sources files
Re-apply data-type changes for apt::source::pin from 1e1baad
Regenerate puppet strings to match.
Remove leading whitespace
Simplify apt::setting logic

Co-authored-by: Tim Meusel <tim@bastelfreak.de>
@amitkarsale
Copy link

is this PR good to be merged?

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.