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

Modified default_pam_*_line to add vas #65

Closed
wants to merge 1 commit into from
Closed

Modified default_pam_*_line to add vas #65

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 7, 2014

changed default_pam_{auth,account,password,session}_lines for Solaris to add VAS

@dantremblay
Copy link

@ghoneycutt - can you merge please?

file { 'pam_system_auth_ac':
ensure => file,
ensure => 'file',
Copy link
Owner

Choose a reason for hiding this comment

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

ensure lines do not need quotes for values of file, directory, link, present, absent, etc.

@ghoneycutt
Copy link
Owner

Commit message is misleading. There are changes to Solaris 9, 10, and 11 in here. This makes it really difficult to merge as you have 1,136 additions and 458 deletions.

Sending a PR that is small in focus so that you are only changing the smallest amount possible in order to get real benefit such as changing VAS values or supporting one new platform make it easy for the upstream developer to understand your change, see that it works, and has appropriate tests.

This commit is just too big and ambitious to fully grok. Suggest using git-rebase and breaking into smaller chunks that are manageable.

@ghost
Copy link
Author

ghost commented Sep 30, 2014

Hi Garrett,

Just break it only to change for Solaris 10. Here is the new PR #78
Will do a new PR for Solaris 11 changes
Thanks

@ghost
Copy link
Author

ghost commented Sep 30, 2014

#79

@ghost ghost closed this Sep 30, 2014
This pull request was closed.
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.

2 participants