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

Add ssl_user_name vhost parameter #2093

Merged
merged 2 commits into from
Jan 4, 2021
Merged

Add ssl_user_name vhost parameter #2093

merged 2 commits into from
Jan 4, 2021

Conversation

bodgit
Copy link
Contributor

@bodgit bodgit commented Nov 20, 2020 •
edited
Loading

Maps to SSLUserName directive.

@bodgit bodgit requested a review from a team as a code owner November 20, 2020 10:28
@puppet-community-rangefinder
Copy link

apache::vhost is a type

Breaking changes to this file WILL impact these 125 modules (exact match):
Breaking changes to this file MAY impact these 33 modules (near match):

This module is declared in 174 of 575 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

ekohl
ekohl reviewed Nov 20, 2020
View reviewed changes
manifests/vhost.pp Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 20, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@14ff1b8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2093   +/-   ##
=======================================
  Coverage        ?   56.36%           
=======================================
  Files           ?       12           
  Lines           ?      220           
  Branches        ?        0           
=======================================
  Hits            ?      124           
  Misses          ?       96           
  Partials        ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14ff1b8...5262a56. Read the comment docs.

@bodgit bodgit changed the title Add ssl_require and ssl_user_name vhost parameters Add ssl_user_name vhost parameter Nov 25, 2020
@bodgit
Copy link
Contributor Author

bodgit commented Nov 25, 2020

Updated to just add the ssl_user_name parameter given SSLRequire is a deprecated directive.

igalic
igalic reviewed Nov 26, 2020
View reviewed changes
Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

🙋🏻‍♀️

spec/defines/vhost_spec.rb Show resolved Hide resolved
Add ssl_user_name vhost parameter
bdaaf97 
Maps to SSLUserName directive.
david22swan
david22swan requested changes Nov 30, 2020
View reviewed changes
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

Thank you for the added functionality, have just reviewed this and while it look's good overall to me I would like to see an acceptance test added to cover the functionality. Doesn't need to be complex, just an instance of it being set:
https://github.com/puppetlabs/puppetlabs-apache/blob/main/spec/acceptance/vhost_spec.rb
If you can do this I would feel happy merging.

@david22swan david22swan self-assigned this Nov 30, 2020
@sanfrancrisko
Copy link
Contributor

sanfrancrisko commented Dec 7, 2020 •
edited
Loading

Thanks for the enhancement @bodgit - as @david22swan mentioned here, if we can get an acceptance test added, then we'd be happy to merge. Please let me know if we can be of any assistance in that regard.

@bodgit
Copy link
Contributor Author

bodgit commented Dec 22, 2020

Acceptance test added, and corrected a small typo.

@sanfrancrisko sanfrancrisko self-assigned this Jan 4, 2021
sanfrancrisko
sanfrancrisko approved these changes Jan 4, 2021
View reviewed changes
Copy link
Contributor

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

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

Thanks for the enhancement @bodgit and the review help @ekohl , @igalic , @david22swan

@sanfrancrisko sanfrancrisko merged commit c78e73f into puppetlabs:main Jan 4, 2021
@bodgit bodgit deleted the ssl-parameters branch January 4, 2021 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igalic igalic igalic left review comments

@ekohl ekohl ekohl left review comments

@david22swan david22swan david22swan requested changes

@sanfrancrisko sanfrancrisko sanfrancrisko approved these changes

Assignees

@david22swan david22swan

@sanfrancrisko sanfrancrisko

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bodgit @codecov-io @sanfrancrisko @ekohl @igalic @david22swan