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

Regenerate code from connie's swagger change #12979

Merged
merged 9 commits into from
Aug 14, 2020

Conversation

seankane-msft
Copy link
Member

Regenerates the _generated file and updates the get_entity method for the new changes.

@@ -5,7 +5,7 @@
# --------------------------------------------------------------------------
import platform
import sys
from ._generated.version import VERSION
from ._generated._version import VERSION

# UserAgent string sample: 'Azure-Storage/0.37.0-0.38.0 (Python CPython 3.4.2; Windows 8)'
# First version(0.37.0) is the common package, and the second version(0.38.0) is the service package
Copy link
Member

Choose a reason for hiding this comment

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

Are we actually using the user-agent string below? This doesn't fit our format:
https://azure.github.io/azure-sdk/general_azurecore.html#telemetry-policy

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we've talked about this partially offline, @seankane-msft were you able to find out a way to not import this and set the x-ms-version on our side, and just rely on the generated code? I think this can also be addressed in a separate PR as long as there's an issue open for it

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not yet, there is an issue out related with the versioning (#12411)

@@ -37,7 +40,7 @@ def __init__(

self.url = url
self.version = "2019-02-02"
kwargs.setdefault('sdk_moniker', 'azuretable/{}'.format(VERSION))
kwargs.setdefault('sdk_moniker', 'table/{}'.format(VERSION))
Copy link
Member

Choose a reason for hiding this comment

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

this too, I think this should be data-tables/

Wondering if we need to regenerate with autorest and specify namespace like,
--namespace=azure.data.tables --package-name=azure-data-tables

@iscai-msft

Copy link
Contributor

@iscai-msft iscai-msft Aug 12, 2020

Choose a reason for hiding this comment

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

@kristapratico yeah the correct sdk moniker should be 'data-tables'. But we shouldn't need to regenerate, since we're overwriting the generated code's sdk moniker here. Instead, the code I linked should be changed to "data-tables".

@seankane-msft can you change this? We tend to keep the user agent in a separate file, that way you can import the constant from the file and use it. TA file example here, and for now, you can just import the user agent constant from that file and set sdk_moniker equal to that

Copy link
Member

Choose a reason for hiding this comment

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

unrelated to user-agent, I think we still need to regen with azure-data-tables otherwise all generated code refs will be off

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah ended up agreeing with you after going through the whole SDK. I don't think it's 100% necessary since it's more for us, but do agree it would look a lot better

Copy link
Member Author

Choose a reason for hiding this comment

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

@kristapratico @iscai-msft I regenerated this, user-agent isn't used anywhere in this sdk, should I move it to it's own file or should I remove it entirely?

Copy link
Member

Choose a reason for hiding this comment

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

You can move it to its own file like how izzy linked above, it's more manageable that way. It looks like the current user-agent string is

azsdk-python-storage-table/2019-07-07 Python/3.8.3 (Windows-10-10.0.19041-SP0)

and it should look like this:

azsdk-python-data-tables/{SDK version} Python/3.8.3 (Windows-10-10.0.19041-SP0)

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

Actually going through the whole PR, i think it twould be better / more clean to regenerate with package-name azure-data-tables and namespace azure.data.tables as @kristapratico suggested. But overall it looks good, some issues that we might want to address in a separate PR though

@@ -5,7 +5,7 @@
# --------------------------------------------------------------------------
import platform
import sys
from ._generated.version import VERSION
from ._generated._version import VERSION

# UserAgent string sample: 'Azure-Storage/0.37.0-0.38.0 (Python CPython 3.4.2; Windows 8)'
# First version(0.37.0) is the common package, and the second version(0.38.0) is the service package
Copy link
Contributor

Choose a reason for hiding this comment

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

Also we've talked about this partially offline, @seankane-msft were you able to find out a way to not import this and set the x-ms-version on our side, and just rely on the generated code? I think this can also be addressed in a separate PR as long as there's an issue open for it

kristapratico
kristapratico previously approved these changes Aug 14, 2020
@seankane-msft seankane-msft merged commit 18c5bb2 into Azure:master Aug 14, 2020
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Aug 17, 2020
…into get_certs_2016

* 'master' of https://github.com/Azure/azure-sdk-for-python: (26 commits)
  Type inference (Azure#12980)
  Add azure-ai-anomalydetector to error code 5 ignored list (Azure#13131)
  Handle missing model properties in Key Vault 2016-10-01 (Azure#13129)
  fix aad test so it runs locally (Azure#13127)
  Suppress errors in anomoly detector for now. (Azure#13141)
  Anomaly Detector V1, track2 generator (Azure#12931)
  [formrecognizer] Add `kind` to FormElement (Azure#13079)
  Regenerate code from connie's swagger change (Azure#12979)
  Update async (Azure#12978)
  Move get_access_conditions to _serialize.py (Azure#13105)
  [Storage][DataLake]Update Min Dependency (Azure#13108)
  Restore user authentication API from 1.4.0b7 (Azure#13070)
  [Storage][FileShare]ChangeLog update (Azure#13103)
  remove locale (Azure#13102)
  [Storage][Blob]ChangeLog Update (Azure#13081)
  Fixed etag bug (Azure#13078)
  [Storage][Blob]Fix live test and if tags bug (Azure#13054)
  [Storage][Blob][Batch]Support batch delete empty blob list (Azure#13029)
  [Storage][Blob][Bug]Support parsing blob url with '/' in blob name (Azure#12619)
  [formrecognizer] add api version enum (Azure#12888)
  ...
@seankane-msft seankane-msft deleted the regenerate branch September 16, 2020 15:26
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.

3 participants