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

Enable LDAP in autofs #163

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Enable LDAP in autofs #163

merged 1 commit into from
Dec 7, 2020

Conversation

coreone
Copy link
Contributor

@coreone coreone commented Jul 30, 2020

Pull Request (PR) description

Enable LDAP as an option when using autofs. This requires being able to modify the following files:

  • /etc/sysconfig/autofs (or /etc/default/autofs on Debian systems)
  • /etc/autofs_ldap_auth.conf

By default, neither of these files is managed by this module to prevent backward compatibility with existing configurations. Management of the files can be turned on individually via the autofs::manage_service_config
and autofs::manage_ldap_auth_conf booleans. Basic spec tests were also added.

@dhollinger
Copy link
Member

@coreone Looks like there are some failing tests, can you look into those?

@coreone
Copy link
Contributor Author

coreone commented Aug 3, 2020

I can look into the failing tests, but they are unrelated to the changes in this PR.

@coreone
Copy link
Contributor Author

coreone commented Aug 3, 2020

@dhollinger I fixed the linting errors I found, but I can't figure out why the tests are failing. I think it's somehow Travis-related. When I run the test and test_with_coveralls rake tasks on my local environment, all the tests pass.

@coreone
Copy link
Contributor Author

coreone commented Sep 3, 2020

@dhollinger Researched this a little. I think the problem here is that facter 2.5 dropped support for OpenSUSE 13.x from the matrix here:

https://github.com/camptocamp/facterdb

So, the only path forward here is probably:

  • Remove OpenSUSE from metadata.json
  • Move OpenSUSE support in metadata.json to 42
  • Set FACTER_GEM_VERSION to stay at < 2.5

For a test, I'm going to set OpenSUSE to 42 to see if that indeed clears up the tests.

@coreone
Copy link
Contributor Author

coreone commented Sep 3, 2020

Tests now pass. The changes were:

  • Update OpenSUSE version to 42 to get around deprecated 13.1 version that is no longer supported by updated versions of facter
  • Update the autofs_version fact to use Facter::Util::Resolution instead of Facter::Core::Execution to fix the Travis checks.

@coreone
Copy link
Contributor Author

coreone commented Sep 4, 2020

@bastelfreak converted erb templates to epp

@bastelfreak bastelfreak changed the title Enable LDAP in autofs Enable LDAP in autofs; Drop Suse 13.1 support; Add Suse 42 support Sep 6, 2020
@coreone
Copy link
Contributor Author

coreone commented Oct 26, 2020

@dhollinger is anything further needed on this?

@ekohl
Copy link
Member

ekohl commented Nov 25, 2020

My apologies, I didn't see this PR. I just merged #166 where I changed it to 15 so at least the tests passed. Please rebase this on current master.

I should also admit that I didn't test it on OpenSUSE.

@coreone coreone changed the title Enable LDAP in autofs; Drop Suse 13.1 support; Add Suse 42 support Enable LDAP in autofs Nov 25, 2020
@coreone
Copy link
Contributor Author

coreone commented Nov 25, 2020

@ekohl This has been rebased. I removed the OpenSuSE version change since 15.2 in your PR fixed the issue.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks! I can't comment too much on the content since I don't use autofs myself. The code itself looks good.

Since @dhollinger did at least a partial review I'll leave it up to him to take a last look. If he has no time, feel free to ping and then I think it can just be merged.

@ekohl ekohl added enhancement New feature or request and removed needs-rebase backwards-incompatible labels Nov 25, 2020
Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

What about some update to README.md about the new features? Also Puppet Strings documentation so that new parameters get added to REFERENCE.md when it's regenerated.

* Fix linting errors
* Update supported OpenSUSE version to fix unit tests
* Update `autofs_version` fact code
* Add documentation to README
* Fix some linting errors in the README
@coreone
Copy link
Contributor Author

coreone commented Dec 1, 2020

@kenyon added a section to the README about the LDAP changes as well as Puppet Strings inline docs. Also cleaned up most of the linting errors in the README while I was in there.

Copy link
Member

@dhollinger dhollinger left a comment

Choose a reason for hiding this comment

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

LGTM

@dhollinger dhollinger merged commit b05431f into voxpupuli:master Dec 7, 2020
@coreone coreone deleted the ldap branch December 7, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants