PagerDuty APIv2 ServiceKey to RoutingKey

[onlynone]

Required for all non-trivial PRs

• Rebased/mergable
• Tests pass
• CHANGELOG.md updated
• Sign CLA (if not already signed)

I think this should address issue #1938

[nathanielc]

@onlynone Thanks this is great!

@russorat Can you validate that this works for you?

[russorat]

@nathanielc we will need to update chronograf alert rule builder for this change.

[onlynone]

@russorat I'm not sure where that is. Would you be able to push a change for that, or point me where to look? Is there test coverage for the builder?

[russorat]

@onlynone @nathanielc is there any way for the chronograf team to know when to use serviceKey vs routingKey when generating tickscripts via the alert rule builder?

[nathanielc]

@russorat Yes, serviceKey is v1 routingKey is v2.

[nathanielc]

I had a chat with @russorat and the issue is we have made an official release that uses the serviceKey function names so we cannot remove it. That said this PR is a better representation of the PagerDuty v2 API.

The solution is to add a serviceKey function to the pager duty handler so that in TICKscript you can still use serviceKey even if that is a misleading name.

Then people can chose to use the better routingKey name when they are ready.

Once that change is in this should be backwards compatible and good to go! Thanks!

[nathanielc]

If you add a ServiceKey function to the PagerDuty2Handler that takes a string and sets the RoutingKey field then we maintain backwards compatibility.

Add a //tick:property comment to the function and a tick:ServiceKey annotation to the RoutingKey field to get it to act like a property function instead of a chaining function.

[onlynone]

@nathanielc I tried making the changes you recommended, but I'm getting the error no method or property "routingKey" on *pipeline.PagerDuty2Handler. This is the diff I came up with based on your feedback and advice:

diff --git a/pipeline/alert.go b/pipeline/alert.go
index 90000f07..72ce5101 100644
--- a/pipeline/alert.go
+++ b/pipeline/alert.go
@@ -1018,7 +1018,14 @@ type PagerDuty2Handler struct {
 
        // The routing key to use for the alert.
        // Defaults to the value in the configuration if empty.
-       RoutingKey string `json:"routingKey"`
+       RoutingKey string `tick:"ServiceKey" json:"routingKey"`
+}
+
+// Allow ServiceKey as backwards compatible way to set the routing key
+// tick:property
+func (pd2 *PagerDuty2Handler) ServiceKey(serviceKey string) *PagerDuty2Handler {
+       pd2.RoutingKey = serviceKey
+       return pd2
 }
 

I'm far from knowledgeable about how all the structs/properties/functions/tick script specification interact. If there's something I'm missing that would allow alert().pagerduty2().routingKey('blah') and alert().pagerduty2().serviceKey('blah') to both work and just update the pipeline.PagerDuty2Handler.RoutingKey property. Feel free to update this PR (I think maintainers have modification access to this PR).

Would it not make more sense to just treat pagerduty2().serviceKey() as a typo of pagerduty2().routingKey() ? In which case it would just be a bug fix, not a feature of the API that needs to be supported forever? All the documentation just mentions .routingKey(). If it had been .routingKee() it would just be fixed without keeping the misspelling, right?

[onlynone]

Hmm, I think if I do this it works:

diff --git a/pipeline/alert.go b/pipeline/alert.go
index 90000f07..358f2ee9 100644
--- a/pipeline/alert.go
+++ b/pipeline/alert.go
@@ -1019,6 +1019,16 @@ type PagerDuty2Handler struct {
 	// The routing key to use for the alert.
 	// Defaults to the value in the configuration if empty.
 	RoutingKey string `json:"routingKey"`
+
+	// tick:ignore
+	_ string `tick:"ServiceKey"`
+}
+
+// Allow ServiceKey as backwards compatible way to set the routing key
+// tick:property
+func (pd2 *PagerDuty2Handler) ServiceKey(serviceKey string) *PagerDuty2Handler {
+	pd2.RoutingKey = serviceKey
+	return pd2
 }
 
 // Send the alert to HipChat.

Does that make sense?

[onlynone]

@nathanielc I think this should allow .serviceKey for backwards compatibility.

[onlynone]

@nathanielc @russorat Do these changes look good? I'd like to get this change merged because I've got another PR I'd like to open that involves pagerduty2, but it would create conflicts with this branch if I based it off master. Right now I've got the other branch on top of this one, so I don't want to open it until this one is resolved somehow.

[nathanielc]

@onlynone That change with the empty field does make sense. Thanks for figuring that out.

The change looks good to me. @russorat Will you be able to test this for backwards compatibility in Chronograf?

[onlynone]

@russorat let me know if there's anything else I can do for this PR.

[onlynone]

@nathanielc @russorat ping?

[russorat]

i will have time to test this tomorrow.

[russorat]

this is fine for chronograf. our alert rule builder doesn't let you override the routingKey in the tickscript anyway. and as long as the api for setting the config is backwards compatible, we are good.

[onlynone]

@nathanielc good to merge?

[onlynone]

@russorat @nathanielc is there someone else who needs to sign off on this?

[onlynone]

@stevebang @russorat @nathanielc it's been 2 months since I pushed the last commit on this PR, which russ and nathaniel said looked good, is there anything else I need to do to get it merged?

[onlynone]

@timhallinflux are there any more steps required to merge this into master now that it's been added to the 1.5.1 milestone?