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

Event grid track 2 #13010

Merged
merged 19 commits into from
Aug 20, 2020
Merged

Event grid track 2 #13010

merged 19 commits into from
Aug 20, 2020

Conversation

lmazuel
Copy link
Member

@lmazuel lmazuel commented Aug 10, 2020

Known issues:

  • sdist validation (MANIFEST.in)
  • offline preparer
  • Py 2.7 urllib parse
  • encoding compatibility with Py2.7

* 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
@lmazuel lmazuel requested a review from kalyanaj as a code owner August 10, 2020 22:06
@lmazuel lmazuel requested review from rakshith91 and KieranBrantnerMagee and removed request for kalyanaj August 10, 2020 22:06
@lmazuel lmazuel added Client This issue points to a problem in the data-plane of the library. Event Grid labels Aug 10, 2020
@lmazuel lmazuel added this to the [2020] September milestone Aug 10, 2020
Rakshith Bhyravabhotla added 2 commits August 11, 2020 12:50
"""
try:
if isinstance(event, six.binary_type):
event = json.loads(event.decode('utf-8'))

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?

Copy link
Contributor

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):
Copy link
Member

@KieranBrantnerMagee KieranBrantnerMagee Aug 13, 2020

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')

Copy link
Contributor

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

from azure.eventgrid import EventGridPublisherClient, EventGridEvent, CloudEvent
from azure.core.credentials import AzureKeyCredential

topic_hostname = "<YOUR-TOPIC-NAME>.<REGION-NAME>-1.eventgrid.azure.net"
Copy link
Member

@KieranBrantnerMagee KieranBrantnerMagee Aug 13, 2020

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__))))

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

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

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.

@@ -0,0 +1,21 @@
# Azure EventGrid Client Generation for Python
Copy link
Member

@KieranBrantnerMagee KieranBrantnerMagee Aug 13, 2020

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?

Copy link
Contributor

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

Rakshith Bhyravabhotla added 2 commits August 17, 2020 23:04
* 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
Rakshith Bhyravabhotla added 2 commits August 18, 2020 10:12
* 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>
@KieranBrantnerMagee
Copy link
Member

/azp run python - eventgrid - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rakshith91 rakshith91 merged commit af30a10 into master Aug 20, 2020
@rakshith91 rakshith91 deleted the event_grid_v2 branch August 20, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Event Grid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants