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

modulesync 5.3.0; Drop Puppet 5 support #303

Closed
wants to merge 11 commits into from
Closed

Conversation

bastelfreak
Copy link
Member

modulesync 4.0.0

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Fails because spec_helper_acceptance needs to be updated to use voxpupuli-acceptance.

@ekohl
Copy link
Member

ekohl commented Nov 25, 2020

That's using the simp helpers so not a trivial conversion. Also not sure if firewalld actually works in containers.

@bastelfreak bastelfreak changed the title modulesync 4.0.0 modulesync 5.3.0 Jun 3, 2022
@bastelfreak bastelfreak changed the title modulesync 5.3.0 modulesync 5.3.0; Drop Puppet 5 support Jun 3, 2022
@bastelfreak
Copy link
Member Author

I disabled acceptance tests for now. this allows us to merge the PR and at least enable unit testing

Copy link
Member

@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.

Looks good, but maybe add "Add support for Puppet 7" to the PR title for the changelog?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Small nit: the commit 9ff87f3 should be titled migrate to rspec-puppet-facts.

Overall no objection to merging this but I agree it should mention adding Puppet 7 support.

@@ -12,11 +14,9 @@
def exists?
builtin = true

found_resource = execute_firewall_cmd(['--get-services'], nil).strip.split(' ').include?(@resource[:name])
found_resource = execute_firewall_cmd(['--get-services'], nil).strip.split.include?(@resource[:name])
Copy link
Member

Choose a reason for hiding this comment

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

Is this really the same? Doesn't this also split on newlines where it previously didn't?

unless @resource[:ports].include?(:unset)
ports_protos = Array(@resource[:ports]).select { |x| x['port'].nil? }.map { |x| x['protocol'] }
end
ports_protos = Array(@resource[:ports]).select { |x| x['port'].nil? }.map { |x| x['protocol'] } unless @resource[:ports].include?(:unset)
Copy link
Member

Choose a reason for hiding this comment

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

I still think the guard cop here produces a too long line so I opened rubocop/rubocop#10708

@@ -26,6 +28,7 @@ def key_val_opt(opt, resource_param = opt)
def eval_source
args = []
return [] unless (addr = @resource[:source])
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but I wonder why this isn't either return args ... or return [] ... and then args = []. Now it's just creating an unused array in some cases.

@trevor-vaughan
Copy link
Collaborator

trevor-vaughan commented Jul 30, 2022

@ekohl firewalld has the ability to run in containers but the containers have to be started privileged with CAP_NET_ADMIN so that the underlying tools can access the napespaced firewall. It's completely hit and miss.

iptables had the ability to work this way but I haven't checked nftables yet. Would recommend keeping the acceptance tests targeting VMs until it can all be confirmed.

trevor-vaughan added a commit that referenced this pull request Aug 15, 2022
@jcpunk
Copy link
Contributor

jcpunk commented Aug 23, 2023

There's been enough drift that it might be worth redoing this PR?

@bastelfreak
Copy link
Member Author

as stated on the mailinglist: I got the impression that the module is abandoned and I've no plans to work further on it. If someone want's to keep using it I suggest someone tries a new modulesync.

@jcpunk jcpunk closed this Aug 23, 2023
@jcpunk jcpunk deleted the modulesync branch August 23, 2023 21:01
@jcpunk
Copy link
Contributor

jcpunk commented Aug 23, 2023

Somehow my subscription to that list got suspended...

Trying in #347

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.

6 participants