-
Notifications
You must be signed in to change notification settings - Fork 2.8k
add custom headers for webhooks, refactor retry logic #419
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
Conversation
…triggers_refactor # Conflicts: # server/src-lib/Hasura/RQL/DDL/Subscribe.hs
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
dea475c to
bd05009
Compare
server/src-lib/Hasura/Events/Lib.hs
Outdated
| value <- getHeaderValue header | ||
| return $ (,) <$> pure name <*> value | ||
|
|
||
| getHeaderName :: HeaderConf -> IO N.HeaderName |
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 need not be in IO
| data HeaderType = FromEnv | FromValue deriving (Show, Eq, Lift) | ||
| $(deriveJSON defaultOptions ''HeaderType) | ||
|
|
||
| data HeaderConf |
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.
data HeaderConf = HeaderConf HeaderName HeaderValue
data HeaderValue
= HVValue Text
| HVEnv TextThe json structure can be as follows:
{
"name" : "Authorization",
"valueFromEnv": "ET_AUTH_KEY"
}or
{
"name" : "X-Source",
"value": "hasura-et"
}|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
server/src-lib/Hasura/Events/Lib.hs
Outdated
| eventId = eId e | ||
| eeCtx <- asks getter | ||
|
|
||
| mheaders <- liftIO $ mapM getHeader headerConfs |
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.
Don't read these in every webhook call? We don't expect them to change once the server starts. So you might as well validate them and store the resolved header in the cache.
c37fbd7 to
1cf8be1
Compare
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
# Conflicts: # server/src-lib/Hasura/Events/Lib.hs # server/src-lib/Hasura/RQL/DDL/Subscribe.hs
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
…rs_headers # Conflicts: # server/src-lib/Hasura/RQL/DDL/Subscribe.hs # server/src-lib/Hasura/RQL/Types/Subscribe.hs
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
1 similar comment
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
f622a84 to
feb6f14
Compare
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
| schemaCachePolicy = SCPReload | ||
|
|
||
| getHeadersFromConf :: [HeaderConf] -> IO [(HeaderName, Maybe T.Text)] | ||
| getHeadersFromConf = mapM getHeader |
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.
We don't want to throw errors if the environment variable is not present?
server/src-exec/Ops.hs
Outdated
|
|
||
| curCatalogVer :: T.Text | ||
| curCatalogVer = "2" | ||
| curCatalogVer = "2.1" |
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.
change it it 3
| phaseTwo q _ = deliverEvent q | ||
| schemaCachePolicy = SCPNoChange | ||
|
|
||
| getHeadersFromConf :: [HeaderConf] -> IO [(HeaderName, Maybe T.Text)] |
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.
Let's throw an error if the environment variable is not present.
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
|
Review app available at: https://hge-ci-pull-419.herokuapp.com |
|
Needs docs |
|
Added |
|
Review app https://hge-ci-pull-419.herokuapp.com is deleted |
Most of our span names were in the format `snake_case` and usually reflected `name_of_the_function`. This is unfriendly, so we've changed it to match the format used by data connectors, which use a format like "Database request" or "Waiting for connection". It would be nice if `Execute request plan for query field` could be `Execute request plan for query field "person"` but it seems changing the currently used `&'static str` for `String` causes all sorts of other lifetime issues, so I suppose we're better including more information as span attributes instead. Therefore, all our span names need to be pretty much static (or at least, dynamically chosen from a list of static names). Note: I have not changed the `SpanVisibility::Internal` span names for now. Most of these reflect function names and this is pretty useful IMO. Happy to change this later if other feel strongly though. V3_GIT_ORIGIN_REV_ID: f2226b2466e8592676f3f4635d483289f0e3f6aa
No description provided.