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

Add templating for MQTT topics #2202

Merged
merged 4 commits into from
Aug 9, 2019

Conversation

fhriley
Copy link
Contributor

@fhriley fhriley commented Apr 22, 2019

This adds the ability to use templates when specifying the MQTT
topic. This is much more useful than static topics since you
can create topics like this:

switches/{{ index .Tags "agent_host" }}/ports/{{ index .Tags "port_num" }}

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

This adds the ability to use templates when specifying the MQTT
topic. This is much more useful than static topics since you
can create topics like this:

switches/{{ index .Tags "agent_host" }}/ports/{{ index .Tags "port_num" }}
@fhriley fhriley force-pushed the mqtt_topic_template branch from 69d6a6e to dc34e23 Compare April 23, 2019 14:16
@fhriley
Copy link
Contributor Author

fhriley commented May 21, 2019

Is anyone looking at these?

@fhriley
Copy link
Contributor Author

fhriley commented Jul 19, 2019

@nathanielc @aanthony1243 Can someone look at this? We are using this in production, but we prefer not to have to maintain a custom build of kapacitor. The static topics that kapacitor supports without this change are essentially useless.

Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

This looks good, there are a few small things that need to change.

@nathanielc
Copy link
Contributor

@fhriley LGTM, can you fix the conflicts and then I can merge.

@fhriley
Copy link
Contributor Author

fhriley commented Aug 8, 2019

Merge conflict resolved.

Copy link
Contributor

@nathanielc nathanielc 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 completely forgot but could you add a test for this? Should be as simple as copy pasting this test case here and then using a template as the topic and validating it works.

@fhriley
Copy link
Contributor Author

fhriley commented Aug 9, 2019

Test added.

Copy link
Contributor

@nathanielc nathanielc left a comment

Choose a reason for hiding this comment

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

Thanks!

@nathanielc nathanielc merged commit f3673ce into influxdata:master Aug 9, 2019
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.

2 participants