sous-chefs / redisio

Development repository for the redisio cookbook
https://supermarket.chef.io/cookbooks/redisio
Apache License 2.0
297 stars 301 forks source link

Add includes option to sentinel config file #491

Closed zygisa closed 7 months ago

zygisa commented 8 months ago

Description

Add includes option to Sentinel configuration file.

Issues Resolved

Currently, there's no way to include custom config files into Sentinel config.

Check List

zygisa commented 8 months ago

Can we get a test added for this change please?

Sure thing! Any suggestions on how to implement this? As far as I can see there are currently no config file related tests in sentinel suite. The same functionality for Redis config is also not covered in tests.

zygisa commented 8 months ago

On further inspection, it seems that the integration tests are not even being executed properly since the verifier in kitchen.yml is set to inspec however all the tests are written for serverspec (unless I'm missing something obvious here). If I check the log of any integration test in this PR (for example this one) I see that no tests are executed:

-----> Verifying <multisentinel-rockylinux-8>...
       Detected alternative framework tests for `serverspec`

Test Summary: 0 successful, 0 failures, 0 skipped
       Finished verifying <multisentinel-rockylinux-8> (0m2.74s).

@damacus @xorima any suggestions on how to proceed with this one? 🙏

damacus commented 8 months ago

Been thinking about this one.

Are you able to converge an instance and check the config manually this time.

We should get the tests fixed up in the next iteration.

zygisa commented 8 months ago

Been thinking about this one.

Are you able to converge an instance and check the config manually this time.

We should get the tests fixed up in the next iteration.

Yeah, I've tested locally and have some functionality built around this feature already. Conducted a quick test now - updated multisentinel test suite to look like this:

  - name: multisentinel
    run_list:
      - recipe[redisio::default]
      - recipe[redisio::enable]
      - recipe[redisio::sentinel]
      - recipe[redisio::sentinel_enable]
    attributes:
      redisio:
        servers:
          - port: 6379
          - port: 6380
        sentinels:
          -
            name: 'cluster'
            sentinel_bind: 0.0.0.0
            sentinel_port: 26379
            includes: ['/etc/redis/test.conf']
            masters:
              -
                name: 'sentinel6379'
                master_name: 'master6379'
                master_ip: '127.0.0.1'
                master_port: 6379
              -
                name: 'sentinel6380'
                master_name: 'master6380'
                master_ip: '127.0.0.1'
                master_port: 6380

Sentinel config looks like this after converge:

[root@dokken /]# cat /etc/redis/sentinel_cluster.conf | tail -n 10
################################## INCLUDES ###################################

# Include one or more other config files here.  This is useful if you
# have a standard template that goes to all redis server but also need
# to customize a few per-server settings.  Include files can include
# other files, so use this wisely.
#
# include /path/to/local.conf
# include /path/to/other.conf
    include /etc/redis/test.conf
zygisa commented 7 months ago

hey @damacus, can we get this merged and released, please? Thanks!

zygisa commented 7 months ago

@damacus @xorima sorry for another ping but I'd really appreciate if this could get merged today. We have a functionality we want to release that depends on this feature. Thanks! 🙏

kitchen-porter commented 7 months ago

Released as: 6.7.1