Add support for ALLOWED_CONTENT_CHECKSUMS #183
Conversation
manifests/init.pp
Outdated
| @@ -188,6 +191,7 @@ | |||
| Stdlib::Fqdn $servername = $facts['networking']['fqdn'], | |||
| Array[Stdlib::Absolutepath] $allowed_import_path = ['/var/lib/pulp/sync_imports'], | |||
| Array[Stdlib::Absolutepath] $allowed_export_path = [], | |||
| Array[String] $allowed_content_checksums = ['sha224', 'sha256', 'sha384', 'sha512'], | |||
There was a problem hiding this comment.
Should this be Array[Enum[…]] to limit possibilities to only known ones? Or would that be to limiting for future pulpcore releases?
There was a problem hiding this comment.
I think that makes sense? we'd need to update it anyways to adjust the default i would think? Its unlikely to be a frequent occurrence either :)
There was a problem hiding this comment.
If pulpcore releases a new supported checksum, instead of say puppet-foreman_proxy_content updating what it passes to this parameter to include that new parameter, a release of this module will have to be made to update the list of allowed values to then allow the update to puppet-foreman_proxy_content as a consumer. It is balancing specifying only "valid" values and future proofing. Does Pulp throw an error if an incorrect value is supplied?
There was a problem hiding this comment.
You get an error on startup of the workers:
django.core.exceptions.ImproperlyConfigured: ALLOWED_CONTENT_CHECKSUMS may only contain algorithms known to pulp - see constants.ALL_KNOWN_CONTENT_CHECKSUMS for the allowed list.
Also, sha256 is required, it won't startup without, not sure if we should handle that here. @ehelms you bring up a good point, maybe we should drop the Enum here and only have it in the foreman_proxy_content change?
There was a problem hiding this comment.
I lean towards not having the Enum personally to allow easier future proofing. This is usually where @ekohl shows up and sets us all straight :)
There was a problem hiding this comment.
If you want to reuse it, you can define it a type alias. https://github.com/theforeman/puppet-foreman_proxy/blob/master/types/listenon.pp is probably the easiest example from our codebase. So you would end up with
type Pulpcore::ContentChecksums = Array[Enum[...], 1]Then you can use, both in this module and puppet-foreman_proxy_content, Pulpcore::ContentChecksums $allowed_content_checksums. Benefits are that you only need to maintain it in one place while still having strong validation.
https://rspec-puppet.com/documentation/type_aliases/ documents how to write simple tests for it.
It's not that often that a new checksum type is added and it needs actual code on the Pulp side. Given that there is a list of constants, I do lean to an enum. Especially if we plan to expose this to users. Debugging a failed service start is a much worse experience than an up front error.
|
note that these are the pulp defaults. Katello will likely want to default to allowing 'sha1' as well. Going to open a pr to foreman_proxy_content to do that and make it user configurable |
|
If there is an existing Redmine for the issue, could you please update the commit message with it? This one could be "Refs #xxxxx - Add support for ALLOWED_CONTENT_CHECKSUMS" and the puppet-FPC PR could be "Fixes #xxxxx - expose pulpcore allowed_content_checksums" |
manifests/init.pp
Outdated
| @@ -188,6 +191,7 @@ | |||
| Stdlib::Fqdn $servername = $facts['networking']['fqdn'], | |||
| Array[Stdlib::Absolutepath] $allowed_import_path = ['/var/lib/pulp/sync_imports'], | |||
| Array[Stdlib::Absolutepath] $allowed_export_path = [], | |||
| Array[Enum['md5', 'sha1', 'sha224', 'sha256', 'sha384', 'sha512']] $allowed_content_checksums = ['sha224', 'sha256', 'sha384', 'sha512'], | |||
There was a problem hiding this comment.
I did suggest to extract the Array[Enum[]] to a type alias. That makes it easy to reuse in another module.
There was a problem hiding this comment.
ahh, thanks for clarifying, will add that
There was a problem hiding this comment.
I should have linked back to #183 (comment) for that suggestion.
jlsherrill
force-pushed
the
checksums
branch
2 times, most recently
from
db93ec3 to
3b69539
Compare
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
|
If this looks good i'll update the foreman proxy content pr to match |
|
yes, its supported all the way back to 3.7: https://github.com/pulp/pulpcore/blob/3.7/pulpcore/app/settings.py#L235 |
|
and if you want to wait for #184 for a release, i think that makes sense |
|
And now nightly fails with Why?! |
|
Can we band-aid it somehow? |
No description provided.