-
Notifications
You must be signed in to change notification settings - Fork 8
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
Adding idmap config param #37
Conversation
Thanks for the PR. Can you please provide some context for this PR for me to understand this change. |
Our AD environment requires us to set some of the idmap configs from samba. Here's a link to the samba documentation about idmap config https://wiki.samba.org/index.php/Idmap_config_ad. Here is an example hiera config that we want to try using:
Here is what we have in our smb.conf file that we are trying to replicate with this module.
|
Thanks for providing the explanation. Can you please fix the failing tests? |
I am trying to fix the tests but having some difficulties. I also think there is an underlying issue with the tests currently, see my other PR #36 |
I'm having issues getting my changes to pass the tests. I think I need to add a hash option to the /spec/defines/option_spec.rb file but I'm not 100% sure. I've never dealt with spec tests before and any help would be greatly appreciated. |
@rehanone do you have any advice on adding a hast option to the /spec/defines/option_spec.rb file? |
Please take a look at the build logs. You can run these tests locally using this command |
@@ -45,5 +45,13 @@ | |||
'obey pam restrictions': value => $samba::obey_pam_restrictions; | |||
} | |||
|
|||
$samba::idmap_config.each | $idmap_domain, $idmap_options | { | |||
$idmap_options.each | $idmap_option, $idmap_value | { | |||
samba::option { "idmap config ${idmap_domain}: ${idmap_option}": |
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.
I think there should be a space before the colon here. According the the smb.conf man page, all of the 'idmap config' examples contain spaces before and after the colon.
idmap config * : backend = tdb
idmap config * : range = 1000000-1999999
idmap config MAIN : backend = rid
idmap config MAIN : range = 5000000-5999999
idmap config TRUSTED : backend = rid
idmap config TRUSTED : range = 6000000-6999999
@@ -101,6 +101,19 @@ | |||
) | |||
} | |||
end | |||
|
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.
@welchnut @rehanone could you explain what's going on here? I'm trying to apply the changes to this pull request and the tests are failing at this point. I don't understand what line 113 is doing.
I understand that we're testing all of the permutations of parameter, "$value" in the samba::option defined type but I don't know where, "changes" comes from as only "lens" is a parameter of the defined type.
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.
It's been awhile since I've looked at this so I could be wrong but I think the issue I was running into is that I don't know how to edit the tests to allow a parameter as a hash. Our samba server has been running using this commit for a few months now and I've been meaning to try to fix the tests to get this merged but haven't had the time to do so.
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.
I don't know how to write tests at all let alone how to tackle a hash. My experience with Augeas is about zero, too. I don't think that "ssl tls" is appropriate here but I could be wrong.
I think it was failing on the old TravisCI build for some reason. Please rebase it on the latest code and see if this issue persists with Github Actions. |
Yes, it still fails. I'm working on a fix but I'm not familiar with how augeas works [yet]. |
This pull request can be closed in favor of #45 which is compatible with the Github actions upgrades. |
@welchnut can you please confirm that this PR is obsolete and can be closed. |
Yeah, should be good to close this. |
No description provided.