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

Fixes consul.agent_service_register which was broken for registering #35419

Merged

Conversation

tegbert
Copy link
Contributor

@tegbert tegbert commented Aug 12, 2016

What does this PR do?

Fixes bugs that won't register service checks when registering a service. It also makes it so that salt states that register services and service checks can adhere to the consul.io documentation.

What issues does this PR fix or reference?

??

Tests written?

No. Testing will require a test fixture comprised of at least 4 machine instances, 3 for the consul server cluster and at lease one consul client. There probably needs to be a plan to test the consul module with the test fixture.

Please review Salt's Contributing Guide for best practices.

service checks.

The function "agent_service_register" in the consul.py module failed
to register service health checks and did not follow the consul.io
documentation for doing so. This patch fixes the code to successfully
register service checks, check for dict keys case-insensitive (because
the absense of keys with the correct case would fail silently), and
more closely follow the documentation at consul.io, see:

https://www.consul.io/docs/agent/http/agent.html#agent_service_register

Here is an example salt state for registering a service and a couple
of health checks, similar to the example in the consul.io documents:

example-service-registration:
  module.run:
    - name: consul.agent_service_register
    - consul_url: "http://localhost:8500"
    - kwargs:
        id: redis1
        name: "redis"
        tags: [master, v1]
        address: "127.0.0.1"
        port: 8000
        EnableTagOverride: false
        check:
          script: "/usr/local/bin/check_redis.py"
          http: "http://localhost:5000/health"
          interval: "15s"

NOTE: the saltstack documentation needs to be updated. It wasn't
correct anyway.
@cachedout
Copy link
Contributor

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Aug 15, 2016
@tegbert
Copy link
Contributor Author

tegbert commented Aug 16, 2016

Doc building by Sphinx is broken. See bug #35399. I'd love to fix the documentation, but need to be able to see what it looks like. Suggestions?

--Tim

Sent from myMail app for Android Monday, 15 August 2016, 01:04AM -06:00 from Mike Place notifications@github.com :

Hi @tegbert Would you just adding some docs? Then we can get this in. Thanks! https://jenkins.saltstack.com/job/PR/job/salt-pr-rs-cent7-n/4312/testReport/junit/integration.modules.sysmod/SysModuleTest/test_valid_docs/

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub , or mute the thread .

@cachedout
Copy link
Contributor

@tegbert No worries. I know what needs to be fixed. I'll fix it up. Thanks!

@cachedout cachedout merged commit f16a307 into saltstack:2015.8 Aug 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants