Skip to content
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

Feature multiple operations webhook #1058

Merged

Conversation

mehrdad-khojastefar
Copy link
Contributor

@mehrdad-khojastefar mehrdad-khojastefar commented Sep 29, 2023

Added support for multiple operations for one handler.
closes: #1057
related: #785

So now you can have handlers like this:
@kopf.on.validate("tempaccess", operation=["CREATE", "UPDATE"])

Also it is backward compatible so you can use it like this too:
@kopf.on.validate("tempaccess", operation="CREATE"

I have run tests using pytest and all of them passed.
I also add this commit to one of our projects and tested it manually. It successfully creates the validating webhook with multiple endpoints when managed option is set.

@nolar let me know it there is anything more that I must do. Thanks.

kosprov and others added 4 commits September 29, 2023 23:39
Signed-off-by: Mehrdad Khojastefar <mehrdadkh.uni+github@gmail.com>
Signed-off-by: Mehrdad Khojastefar <mehrdadkh.uni+github@gmail.com>
Signed-off-by: Mehrdad Khojastefar <mehrdadkh.uni+github@gmail.com>
Signed-off-by: Mehrdad Khojastefar <mehrdadkh.uni+github@gmail.com>
@SmsS4
Copy link

SmsS4 commented Oct 5, 2023

This PR would help us in our web-hooks to remove duplication in our codebase.

@nolar nolar force-pushed the feature_multiple_operations_webhook branch 2 times, most recently from 5031a24 to 612d592 Compare October 8, 2023 21:59
…3.7/3.8

Signed-off-by: Sergey Vasilyev <nolar@nolar.info>
@nolar nolar force-pushed the feature_multiple_operations_webhook branch from 612d592 to 0ea4e4d Compare October 8, 2023 22:05
Signed-off-by: Sergey Vasilyev <nolar@nolar.info>
Copy link
Owner

@nolar nolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this PR! I have made a few adjustments:

list[] is a syntax from Python 3.9+, while Kopf has to support at least Python 3.8 (I will drop 3.7 in #1062 now). It should be List[], not list[] from PEP-585.

Besides, I suggest it to be any collection, be that a list, a tuple, a set, a frozen set, or something self-made. Hence, Collection[] as the most generic class with size & items but no ordering (which would make it a Sequence[]).

And also I prefer to have clear names about the content of that field, so it should be in plural, not singular. I have added operations (plural) everywhere and made operation (singular) supported but deprecated (with a warning — as for some other deprecated names, e.g. namespace/namespaces).

The Operations type is removed as too verbose — it can be expressed with StdLib's typing library + Operation, does not contain complexity like Union[], so let's keep it simple.

@nolar nolar merged commit a4f7848 into nolar:main Oct 8, 2023
28 of 31 checks passed
@mehrdad-khojastefar
Copy link
Contributor Author

I can update the docs and examples as well on weekend.

@mehrdad-khojastefar mehrdad-khojastefar deleted the feature_multiple_operations_webhook branch October 31, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of supporting multiple operations for one admission webhook endpoint
4 participants