-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Webhook storage (fixes #2835) #3000
Merged
Merged
Changes from 7 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
541b1c1
Add WebHook storage (fixes #2835)
jameslamb 9bafd61
update LocalAgent tests
jameslamb 32ff9e3
cleaning up docs
jameslamb 8ca588d
fix typehint and add changes entry
jameslamb b4f5537
tell mypy to trust me
jameslamb e98b62f
add test for uncovered line
jameslamb 1a3b94d
fix typos
jameslamb 849a53f
Merge branch 'master' into feat/webhook-storage
jameslamb 59bcc06
add support for stored_as_script
jameslamb 8c0e32d
WebHook -> Webhook
jameslamb 1b535f3
Merge branch 'master' into feat/webhook-storage
jameslamb 3646336
merge master
jameslamb 495372f
fix tests, merge master
jameslamb 8a2b474
Merge branch 'master' into feat/webhook-storage
jameslamb ba6956a
Merge branch 'master' into feat/webhook-storage
jameslamb 0add059
add _request to keyword args
jameslamb 937dd84
replace secret_config with templating
jameslamb a2d40a1
linting
jameslamb 5d29ff8
Merge branch 'master' into feat/webhook-storage
jameslamb 85a7751
Merge branch 'master' into feat/webhook-storage
jameslamb 921e387
remove deepcopy
jameslamb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
enhancement: | ||
- "Add WebHook storage - [#3000](https://github.com/PrefectHQ/prefect/pull/3000)" | ||
|
||
contributor: | ||
- "[James Lamb](https://github.com/jameslamb)" |
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
I find this kwarg a bit confusing and limiting (it only works on headers? what if you need to set a query parameter or part of the body? what if you need to modify the secret before transmitting (e.g. adding a
Bearer
prefix)). I wonder if we might instead support templating in the (recursive) values inget_flow_kwargs
/build_flow_kwargs
. The semantics might be:build_kwargs
andget_flow_kwargs
for stringsOne way of doing this would be to make use of
string.Template
and a magic mapping to handle dynamically looking up fields. We'd might want to change the regex to drop the$
prefix to make it similar tostr.format
not (or maybe not? not sure what's clearer) but this works. (note thatstr.format
converts the mapping to a dict before formatting, so we can't use that to dynamically load secrets/environment variables unfortunately).Could also use the regex module directly, which might be simpler 🤷.
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.
I also find the
get_flow
andbuild
prefixes on these kwargs a bit off. I know they correspond to the requests for thebuild
andget_flow
methods, but without the wordrequest
in therebuild_kwargs
looks like kwargs to pass tobuild
to me. Feels too tied to the interface method names and not tied to what the requests are actually doing (storing and loading bytes). Perhaps?I'd use
put
andget
, except those conflict with the http methods. Not a strong opinion, just a thought.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.
I thought this was sufficiently complex that I shouldn't make it part of the first pass, but if you think it's necessary I'm happy to add it!
I'm a little worried about free-form templating everything though...that's going to be a problem if you have JSON jammed in a string, like the DropBox API requires (https://github.com/jameslamb/webhook-storage-integration-tests/blob/3bc93bf2ce4b9a0539306045f2f6a82bc3325c53/test-dropbox.py#L47). That opens you up to needing to know how to escape the right
}
, which doesn't sound fun.maybe it would be simpler to, instead of templating individual string fields, just allow people to replace the entire value of any build_kwarg or get_flow_kwarg with the value of an environment variable / secret?
I did think about this specific case. If your API token's literal value is
abc
, there's no reason you couldn't putBearer abc
into an environment variable / secret, right? Without needing to have any code run to addBearer
to the front.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.
It feels non-intuitive to me to require storing a full authorization header/url/etc... in a secret to make proper use of it. If we keep the
$
prefix requirement thatstring.Template
uses, that (I believe) avoids the issue of accidentally templating things that just happen to contain{}
characters, since they won't match the regex.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.
ooooo ook! I like that a lot. I'll add that to this PR.
As for the names, I feel that there's value in coupling to the method names actually.
build()
andget_flow()
are important to understand when using a Storage object, I think, and I'd rather couple to those than invent another thing people have to reason about. But I do like adding_request
to make that clearer. How do you feel aboutbuild_request_*
andget_flow_request_*
?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.
Makes sense to me!
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.
Ok I just attempted the templating thing. Awesome suggestion, I like this a lot better than the
secret_config
approach.Commit: 937dd84
I also updated my integration tests and ran them to be sure it's working as expected: https://github.com/jameslamb/webhook-storage-integration-tests.
Note for reviewers
I think it could be valuable to offer more explicit control over whether environment variables or Prefect secrets are used, to avoid issues caused by conflicting names.
I think that could be done in a backwards-compatible way in the future, by adding a
render_preferences
argument that is like{"SOME_VARIABLE": "environment"}
, which changes the behavior for rendering${SOME_VARIABLE}
from "env --> secret --> error-if-absent" to "env --> error-if-absent".I thought that complexity wasn't worth it for a first pass, but I'd be happy to add something like it if reviewers think it's a good idea.