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

Add template for performing custom sshd pam config #141

Merged
merged 1 commit into from
Aug 29, 2016

Conversation

boandersson
Copy link
Contributor

Specific use case is to be able to supply options to pam_vas3 that are
specifically needed (and should only be configured) for sshd to resolv
intermittent ssh login failures with QAS.

@ghoneycutt
Copy link
Owner

Thanks @boandersson

@Phil-Friderici what do you think?

@@ -0,0 +1,15 @@
# This file is being maintained by Puppet.
Copy link
Owner

Choose a reason for hiding this comment

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

we probably need some fixtures around this for the spec testing

Copy link
Contributor

Choose a reason for hiding this comment

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

True if the sequence does matter here. Looking at the short result of the existing tests I would recommend to use a variable instead of adding a a fixture file. Like here [1].

[1] https://github.com/ghoneycutt/puppet-module-bind/blob/master/spec/classes/init_spec.rb#L127-L149

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree that the current tests aren't that useful, esp since the sequence of each section is very important... I'll have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took the route with a variable as suggested by @Phil-Friderici , let me know what you think

@ghoneycutt
Copy link
Owner

@boandersson would it be possible to just update the default VAS entries in the code?

@Phil-Friderici
Copy link
Contributor

Phil-Friderici commented Aug 17, 2016

An alternative approach would be to bring all sshd.$osfamily.erb template content into the code. This would become a bigger refactoring of the functionality. Which I would only do on a major version update.

So I can understand why @boandersson chose this way.

@ghoneycutt
Copy link
Owner

I'm going to defer to @Phil-Friderici on this. When he gives the thumbs up, I'll merge and release.

@Phil-Friderici
Copy link
Contributor

@boandersson Think we only need to agree how detailed we want to test here. After that, please squash all commits into one for a nice commit history :)

@boandersson
Copy link
Contributor Author

Added test updates from @Phil-Friderici. Let me know what you think before I squash any commits

@Phil-Friderici
Copy link
Contributor

From my perspective this is complete.
@boandersson please squash it into one commit.
@ghoneycutt please have a second look and merge it if you are ok with it too.

Specific use case is to be able to supply options to pam_vas3 that are
specifically needed (and should only be configured) for sshd to resolv
intermittent ssh login failures with QAS.
@Phil-Friderici
Copy link
Contributor

@boandersson thanks for squashing
@ghoneycutt looks good to me now :)

@ghoneycutt
Copy link
Owner

Thank you both!

@ghoneycutt ghoneycutt merged commit e6efbc6 into ghoneycutt:master Aug 29, 2016
@ghoneycutt
Copy link
Owner

Released in v2.27.0

@boandersson
Copy link
Contributor Author

Thanks for code review and merge @ghoneycutt !

@boandersson boandersson deleted the custom_sshd_config_rebase branch August 30, 2016 06:44
@boandersson boandersson restored the custom_sshd_config_rebase branch August 30, 2016 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants