-
Notifications
You must be signed in to change notification settings - Fork 258
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
Change PagerDuty2 service-key
to routing-key
#3450
Conversation
service-key
to routing-key
|
||
private handleRoutingKeyRef = r => (this.routingKey = r) | ||
|
||
private handleEnabledChange = (e: ChangeEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider typing the return as void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. just copied all of this -- will type :)
this.disableTest() | ||
} | ||
|
||
private handleSubmit = async e => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider typing function signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
private handleSubmit = async e => { | ||
e.preventDefault() | ||
|
||
const properties = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be type Properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
onGoToConfig, | ||
validationError, | ||
}) => | ||
selectedHandler.enabled ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing this to not use a ternary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
<span className="icon cog-thick" /> | ||
{validationError | ||
? 'Exit this Rule and Edit Configuration' | ||
: 'Save this Rule and Edit Configuration'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few places that could be more typed and a place where a ternary is used. Other than that, looks good!
f6a46c6
to
13b6bda
Compare
Closes #2786
What was the problem?
PagerDuty v2's integration key is
routing-key
, notservice-key
.What was the solution?
Create separate, dedicated PagerDuty2 config & handler components with the correct
routing-key
field name and UI copy.