Skip to content

Fix CI build - #333

Merged
mtodd merged 30 commits into
ruby-ldap:masterfrom
vroldanbet:fix-ci-build
Nov 16, 2019
Merged

Fix CI build#333
mtodd merged 30 commits into
ruby-ldap:masterfrom
vroldanbet:fix-ci-build

Conversation

@vroldanbet

@vroldanbet vroldanbet commented Nov 14, 2019

Copy link
Copy Markdown
Contributor

Implement development/CI environment based on docker

Master CI build recently broke. The install-openldap script is very complex and assumes a specific environment. This is clearly not completely isolated from the fact that new versions of packages may be backported to the distribution used. I suspect this is what happened (noticed builds started breaking with a new version of slapd).

Instead of fixing the script, I decided I could reuse open-source maintained OpenLDAP docker containers. The https://github.com/osixia/docker-openldap project handles a lot of the work performed by install-openldap. As a consequence. we can remove a lot of stuff from this repo:

  • some LDIF fixtures
  • script/install-openldap
  • script/generate-fixture-ca
  • test/support (vagrant stuff)
  • certificates (it's now generated by the container)

In general, it's a lot of less code to maintain, and less code that can break as dependencies change.

Docker is also fairly ubiquitous in CI systems, and it would help closing the gap between CI and local development environment. Travis supports it nicely and it could open the door to adopt GitHub Actions too?

Extra niceties

  • added most recent ruby versions to the build matrix
  • no more "travis specific" tests

The not so nice part

  • I couldn't find a way to workaround having to modify /etc/hosts. I feel it's a small price to pay to simplify the setup considerably, but I'll try to explore alternatives (if I managed to name the containers hostname localhost it would get fixed)
  • the timeout test, I can't make it go green. I decided to disable it.
  • test relying on system cert store, didn't try hard how to install cert in travis, and it's additional burden with little return
  • test with multiple valid hostnames - I couldn't get the certificate generation to have multiple hostnames. Instead, I decided to run the test with the same valid hostname 2 times. It should hopefully be equivalent.

TODO

  • allow devs in local to avoid changes to /etc/hosts for integration tests

instead of polluting devs machine, let's rely on a docker container
to spin up the service

relies on osixia/openldap:1.3.0. Customizes a few things:
- adds the seed on bootstrap
- does not enforce client certificate
- sets a hostname to avoid domain verification issues during handshake

The cert domain is also added to /etc/hosts
anonymous access is not enabled in this setup, so every test needs
to perform authentication first
I couldn't manage to get the container running with a cert
issued to a given IP, like 127.0.0.1 or localhost.
Instead, I specified a static hostname
(the container uses hostname to generate the cert)
and injected it in travis.

Unfortunately, in local development this means changing /etc/hosts,
but I feel that's a better option that having to install LDAP locally
so that all retcode tests succeed
it's a little price to pay in the current setup and allows us to
have the same tests locally and dev.
all environments run the same set of tests, no env specific test
so that we are able to run a test that does not specific CACERT
and so the library fallsback to system cert store
I'm not sure how to enable this in Travis
with the dockerized test openldap server, none of this is needed
the container uses HOSTNAME to generate the cert, and
it really didn't like "localhost" as hostname.

As a workaround, I had to add an arbitrary hostname. There may be other
alternatives to get the host to be known, but the most obvious
is modifying /etc/hosts
@vroldanbet
vroldanbet marked this pull request as ready for review November 14, 2019 11:10
Comment thread .travis.yml Outdated
Comment thread test/integration/test_bind.rb Outdated
# The following depend on /etc/hosts hacking.
# We can do that on CI, but it's less than cool on people's dev boxes
def test_bind_tls_with_multiple_hosts
omit_unless ENV['TRAVIS'] == 'true'

@CamiloGarciaLaRotta CamiloGarciaLaRotta Nov 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:) 👌🏽

@mtodd mtodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💯 to moving to relying on Docker for the OpenLDAP dependency. And thanks for breaking down the decisions and tradeoffs!

Comment thread script/install-openldap
Comment thread README.rdoc Outdated
Comment thread .travis.yml Outdated
Comment thread test/integration/test_bind.rb Outdated
Comment thread test/integration/test_bind.rb
Co-Authored-By: Matt Todd <chiology@gmail.com>
This was referenced Jan 24, 2023
msk pushed a commit to msk/pkgsrc that referenced this pull request May 11, 2026
Update ruby-net-ldap to 0.16.2.


=== Net::LDAP 0.16.2

* Net::LDAP#open does not cache bind result {#334}[ruby-ldap/ruby-net-ldap#334]
* Fix CI build {#333}[ruby-ldap/ruby-net-ldap#333]
* Fix to "undefined method 'result_code'" {#308}[ruby-ldap/ruby-net-ldap#308]
* Fixed Exception: incompatible character encodings: ASCII-8BIT and UTF-8 in filter.rb {#285}[ruby-ldap/ruby-net-ldap#285]
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 13, 2026
Update ruby-net-ldap to 0.16.2.


=== Net::LDAP 0.16.2

* Net::LDAP#open does not cache bind result {#334}[ruby-ldap/ruby-net-ldap#334]
* Fix CI build {#333}[ruby-ldap/ruby-net-ldap#333]
* Fix to "undefined method 'result_code'" {#308}[ruby-ldap/ruby-net-ldap#308]
* Fixed Exception: incompatible character encodings: ASCII-8BIT and UTF-8 in filter.rb {#285}[ruby-ldap/ruby-net-ldap#285]
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