-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow additional settings for GSSAPI in Vhost #2215
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
Conversation
@tuxmea Sorry for the wait on review. Anyway this change look's good to me but could you also update the relevant documentation linked below? Would be sad if you put in a great change like this and nobody could find it:
|
@tuxmea Sorry to come back but while reviewing other prs I noticed that a similar change had been pushed up by another contributor that contains part of the functionality that you yourself have added. Just felt I should alert you as depending on which PR is merged first you may need to rebase your work. |
In relation to the comment above, have discovered two more PRs that are adding similar functionality to this, one of which was in a good enough state to be merged: And another which will require some more work: |
@tuxmea Hey, just giving an update but on of the PRs I previously mentioned has been merged in so that now |
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.
There is now a merge conflict. While some more parameters have been added, at least AuthName
and AuthType
have not so there's still value in this PR. Please rebase to resolve the conflicts.
397400d
to
5f0787f
Compare
Adding config items: - allowedmech - authname - authtype - basicauth
5f0787f
to
75f6854
Compare
@david22swan @ekohl many thanks for your feedback. PR is rebased and ready to get merged. I also updated the documentation in manifests/vhost.pp |
Failed test for OracleLinux 6:
|
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.
The spec failure looks odd. I'm not sure how this PR could have caused that and we also see it in nightly: https://github.com/puppetlabs/puppetlabs-apache/runs/7381259870?check_suite_focus=true (that said, it is still odd that in Puppet 6 it fails, but doesn't in 7).
Adding config items: