Make a separate hook for interacting with the Pagerduty Events API#18784
Merged
Conversation
GuidoTournois
force-pushed
the
pagerduty_tokens
branch
from
October 6, 2021 19:57
51536e1 to
8138eee
Compare
uranusjr
reviewed
Oct 6, 2021
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
GuidoTournois
force-pushed
the
pagerduty_tokens
branch
from
October 8, 2021 14:49
580550c to
4190333
Compare
GuidoTournois
marked this pull request as ready for review
October 8, 2021 14:51
potiuk
reviewed
Oct 10, 2021
| :param integration_key: PagerDuty Events API token | ||
| :param pagerduty_conn_id: connection that has PagerDuty integration key in the Pagerduty API token field | ||
| """ | ||
|
|
Member
There was a problem hiding this comment.
Should we create a separate "pagerduty_events" type of connection ? Seems that those two different connection types have two different keys ? This is perfectly fine to define second type of connection for the the new hook.
Contributor
Author
There was a problem hiding this comment.
Thanks for the feedback! Yes indeed, the two different hook required different types of api keys.
I think we have a few options:
- keep it as is (probably causes some confusion to airflow users).
- a single connection type with two separate fields for the two tokens. The password field can be renamed as one token, and we can either rename an existing field or use an extra field for the other token. However, in that case only one of the tokens would behave as a password (as is automatically hidden from logs). Thinking about it, that is the current situation as well with the routing_key in the extra field.
- two separate connection types as you mentioned.
I think I would agree with you. Better to be explicit and have both token be hidden from logs.
Contributor
Author
There was a problem hiding this comment.
I have pushed the changes to make it into two separate connection types
54 tasks
91 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to make use of the PagerDuty Events API, you need to provide two API keys in the PagerdutyHook connection, even though one of them is not required for the Events API.
The proposed setup is two have two separate hooks, one for accessing the general Pagerduty API, the other hook for interacting with the EventsAPI.
So I made the new PagerdutyEventsHook, which sole purpose is to interact with the Pagerduty Events API. However, in order to be back compatible the original PagerdutyHook still contains a method to interact with the EventsAPI, which under the hood makes use of the other hook.
closes: #18771