-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Event grid track 2 #13010
Event grid track 2 #13010
Conversation
* genereated python client for event grid * updated readme to use track2 generator * added sas key auth sample * added consume sample, not final * removing old eg * added track 2 changes needed for api view * add consumer operations * cleanup client versions for api view * remove _initialize_mapping() in BaseEventType class in models * track 2 design manual changes * added publisher wrapper in azure/eventgrid for demo * modified naming for publish sample for demo * final sample fix for demo * added response to publish_events(), still need to fix * added decoder for apiview * renamed consumer, added Deserialized/CustomEvent * final for Board Review * testing changes * added EventGridSharedAccessSignatureCredential,Policy and modified samples * added eg_client test * moving generated code out from event_grid_publisher_client nested folder * added consumption function samples * removed eg required package and removed service bus dependency in consumer * removed unnecessary functions * changed consumer deserialize_event() to take,return single event of one type of (string, bytes string, dict). changed DeserializedEvent to have to_json() method, instead of extending DictMixin * added publish tests * fixed PR, added CustomEvent, added tests/samples * updated swagger, removed unnecessary imports * removed unnecessary reqs in dev_requirements * changed async publisher import path, added type hints * modified typehints for publishers, based on apiview * added newlines * added shared_reqs file * moved shared_requirement * fixed non live test * added changelog, test fix * changed topic preparer * added samples to exclude to setup.py
* Fix Tests * other fixes
* other fixes * p2 compat
* other fixes * auto update
""" | ||
try: | ||
if isinstance(event, six.binary_type): | ||
event = json.loads(event.decode('utf-8')) |
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.
Should we allow encoding to be provided?
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.
can be a kwarg
# -------------------------------------------------------------------------------------------- | ||
# pylint:disable=protected-access | ||
|
||
class DictMixin(object): |
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.
@lmazuel I wish we had like, "python_sdk_utils" that contained generic helpers like this, since I'm 100% sure this exists in multiple of our libs.
(I realize this statement sounds like airy musings and not something actionable, so let me rephrase: "is this an insane proposition to you?")
(@rakshith; don't let my contemplation here spook you, code's fine, no changes needed specifically here, just gave me an opportunity to consider 'broader questions')
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.
might be something that can move to azure core
sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_publisher_client_async.py
Outdated
Show resolved
Hide resolved
from azure.eventgrid import EventGridPublisherClient, EventGridEvent, CloudEvent | ||
from azure.core.credentials import AzureKeyCredential | ||
|
||
topic_hostname = "<YOUR-TOPIC-NAME>.<REGION-NAME>-1.eventgrid.azure.net" |
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.
Typically pluggables within samples are os.environ loaded. This is low-pri however, use your judgement for if you want to change these now, or if you feel these samples might change a lot, add an issue to the backlog to make sure this is dealt with "sooner or later"
import os | ||
|
||
PACKAGE_PARENT = '..' | ||
SCRIPT_DIR = os.path.dirname(os.path.realpath(os.path.join(os.getcwd(), os.path.expanduser(__file__)))) |
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.
So I think I know why this is here, and it's kinda a neat idea since running these samples ad-hoc (given the fact that they use real-package-paths from a subdir) often requires making sure you're in a proper env/path to actually resolve these imports can sometimes be a stumbling block. however, I worry that if we start linking these samples to users, they'll be like "why would I have that."
Assuming these samples can be run as "self-contained-one-off-scripts" within a properly configured environment without this path, I'd probably yank this; although I feel bad saying this since it is a nice dev convenience layer. Let me know if you have any clever ways to have our cake and eat it too, or if I'm missing something obvious.
.vscode | ||
local.settings.json | ||
test | ||
.venv |
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.
do we want this to be the .funcignore everyone has? it seems a tiny bit special cased (ala .venv/test, but that may be intentional)
@@ -0,0 +1,130 @@ | |||
# Byte-compiled / optimized / DLL files |
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 is a massive gitignore. maybe just a copy of our core one? which would make me question why we need it at the leaf here. as with .funcignore wondering if we can be targeted if these are required; but also as with funcignore, this is very low pri so if it becomes a rathole feel free to punt on it.
...ure-eventgrid/samples/consume_samples/e2e_samples/functionsapp/EventGridTrigger1/__init__.py
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,21 @@ | |||
# Azure EventGrid Client Generation for Python |
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'm paranoid since Mayuri was not pleased when the SB swagger found its way into Master; @lmazuel Is having this generation readme here kosher to your eyes?
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.
agree - this is misleading - changing it
* other fixes * auto update * Update sdk/eventgrid/azure-eventgrid/CHANGELOG.md * Update sdk/eventgrid/azure-eventgrid/azure/eventgrid/_helpers.py * Update sdk/eventgrid/azure-eventgrid/CHANGELOG.md * Update sdk/eventgrid/azure-eventgrid/azure/eventgrid/_version.py * comments
* other fixes * auto update * Update sdk/eventgrid/azure-eventgrid/CHANGELOG.md * Update sdk/eventgrid/azure-eventgrid/azure/eventgrid/_helpers.py * Update sdk/eventgrid/azure-eventgrid/CHANGELOG.md * Update sdk/eventgrid/azure-eventgrid/azure/eventgrid/_version.py * comments * tests fix
Co-authored-by: KieranBrantnerMagee <kibrantn@microsoft.com>
sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_publisher_client_async.py
Show resolved
Hide resolved
sdk/eventgrid/azure-eventgrid/azure/eventgrid/aio/_publisher_client_async.py
Show resolved
Hide resolved
/azp run python - eventgrid - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
Known issues: