-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
fix puppet-lint warnings / switch to facts hash / drop legacy gluster versions 4.0 and older #203
base: master
Are you sure you want to change the base?
Conversation
@@ -38,6 +38,8 @@ | |||
'4.0' => '55F839E173AC06F364120D46FA86EEACB306CEE1', | |||
'4.1' => 'EED3351AFD72E5437C050F0388F6CDEE78FA6D97', | |||
'^5\.(\d)+$' => 'F9C958A3AEE0D2184FAD1CBD43607F0DC2F8238C', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'^5\.(\d)+$' => 'F9C958A3AEE0D2184FAD1CBD43607F0DC2F8238C', | |
default => 'F9C958A3AEE0D2184FAD1CBD43607F0DC2F8238C', |
f76482e cleans it up a bit further. |
@@ -74,7 +74,7 @@ | |||
|
|||
$_transport = "transport ${transport}" | |||
|
|||
if $options and ! empty( $options ) { | |||
if ! empty( $options ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later there's another if $_options. Perhaps you can combine those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave this pull request as it is, because many others depend on it and we need a working CI pipeline.
) inherits gluster::params { | ||
|
||
# if we manage the repository, we also need a GPG key | ||
if $repo { | ||
assert_type(Variant[Stdlib::Absolutepath, Stdlib::HTTPSUrl], $repo_key_source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about this pattern. It's hard to discover from reading the reference. Should the parameter be Optional[Variant[Stdlib::Absolutepath, Stdlib::HTTPSUrl]]
? In that case maybe this could be simplified to assert_type(NotUndef, $repo_key_source)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the resource ordering, but I think this is redundant code. The value of $repo
and $repo_key_source
is already defined at manifests/params.pp
and we check the type at manifests/repo.pp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's redundant because manifests/repo.pp
already checks it. That's a better approach than duplicating it here.
Hi, just asking, is there any chances this reaching official repo soon? May I help somehow? |
Dear @bastelfreak, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
2 similar comments
Dear @bastelfreak, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
Dear @bastelfreak, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
I've rebased this in https://github.com/runejuhl/puppet-gluster/tree/bugfix, but I noticed that 20decb5 removes a bunch of class specs -- is that in error or is it deliberate? |
Pull Request (PR) description
This Pull Request (PR) fixes the following issues