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.

Sign up for GitHub

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

Jump to bottom

Change the way SELinux is applied for portnumbers #135

Merged
merged 1 commit into from
Sep 28, 2020
Merged

Change the way SELinux is applied for portnumbers #135

merged 1 commit into from
Sep 28, 2020

Conversation

ralfbosz
Copy link
Contributor

@ralfbosz ralfbosz commented Jun 20, 2019 •
edited
Loading

When declaring serveral http_ports with different IP's, but the same portnumber a resource conflict would arise on the selinux-part. Changed the selinux::port to an "ensure_resource", hence fixing issue #120.

@traylenator traylenator added the enhancement New feature or request label Jun 20, 2019
@traylenator
Copy link
Contributor

Hi @ralfbosz the subject of the pull request could be more precise. It sounds a lot more alarming than it currently is.

@ralfbosz ralfbosz closed this Jun 20, 2019
@ralfbosz ralfbosz reopened this Jun 20, 2019
@traylenator
Copy link
Contributor

Tests failed due to failed download - rerunning.

traylenator
traylenator reviewed Jun 24, 2019
View reviewed changes
manifests/http_port.pp Outdated Show resolved Hide resolved
@ralfbosz ralfbosz force-pushed the ensure_resource branch 2 times, most recently from 2393e92 to 142fed8 Compare June 24, 2019 11:05
@traylenator traylenator changed the title Change the way SELinux is applied for portnumbers Wrap selinux::ports with ensure_resource to allow duplicates Jun 24, 2019
@traylenator
Copy link
Contributor

@ralfbosz happy with that title? More useful in the eventual changelog I think.

@ralfbosz ralfbosz changed the title Wrap selinux::ports with ensure_resource to allow duplicates Change the way SELinux is applied for portnumbers Jun 25, 2019
@ralfbosz
Copy link
Contributor Author

Adjusted the commit title...

@traylenator
Copy link
Contributor

From the review docs:

Write the Pull Request Title in a way that users can easily identify if they are impacted or not

@ralfbosz ralfbosz closed this Oct 7, 2019
@ralfbosz ralfbosz reopened this Oct 7, 2019
@vox-pupuli-tasks

This comment has been minimized.

Sign in to view
1 similar comment
@vox-pupuli-tasks
Copy link

Dear @ralfbosz, 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

@vox-pupuli-tasks

This comment has been minimized.

Sign in to view
@vox-pupuli-tasks

This comment has been minimized.

Sign in to view
@ekohl
Copy link
Member

ekohl commented Feb 14, 2020

@alexjfisher is ensure_resource a legacy thing? I thought create_resources was, but ensure is to avoid duplicate definitions.

ekohl
ekohl requested changes Feb 14, 2020
View reviewed changes
manifests/http_port.pp Outdated
@@ -50,13 +50,13 @@
order => "30-${order}",
}

if $facts['os']['selinux'] == true {
selinux::port{"selinux port squid_port_t ${_port}":
if $facts['selinux'] == true {
Copy link
Member

Choose a reason for hiding this comment

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

Please continue to use the modern facts

Suggested change
if $facts['selinux'] == true {
if $facts['os']['selinux'] == true {

@alexjfisher
Copy link
Member

@alexjfisher is ensure_resource a legacy thing? I thought create_resources was, but ensure is to avoid duplicate definitions.

ensure_resource is pretty hacky, but unlike create_resources I don't think there's a new Puppet 4+ alternative.

@vox-pupuli-tasks
Copy link

Dear @ralfbosz, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs?
If you need any help, you can reach out to us on our IRC channel voxpupuli on Freenode or our Slack channel voxpupuli at slack.puppet.com.
You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@alexjfisher
Copy link
Member

@ralfbosz What's the status of this? There's been interest on Slack in getting this merged.

@granquet granquet mentioned this pull request Sep 24, 2020
Closed
@ralfbosz ralfbosz force-pushed the ensure_resource branch 15 times, most recently from 715407f to 6848fdf Compare September 28, 2020 11:19
@ralfbosz
Copy link
Contributor Author

Fixed it...

ekohl
ekohl requested changes Sep 28, 2020
View reviewed changes
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.

Minor detail inline, but overall 👍

manifests/http_port.pp Outdated Show resolved Hide resolved
@ralfbosz
Workaround for duplicate resource http_port voxpupuli#120
bab2643 
The module is now able to handle multiple server
declarations for the same port on different IPs.
ekohl
ekohl approved these changes Sep 28, 2020
View reviewed changes
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.

Travis is still running, but the code looks good.

@bastelfreak bastelfreak merged commit a896606 into voxpupuli:master Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traylenator traylenator traylenator left review comments

@bastelfreak bastelfreak bastelfreak approved these changes

@ekohl ekohl ekohl approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ralfbosz @traylenator @ekohl @alexjfisher @bastelfreak @dhoppe