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

BREAKING CHANGE: Adl object refactor #1324

Merged
merged 11 commits into from
Aug 17, 2017
Merged

BREAKING CHANGE: Adl object refactor #1324

merged 11 commits into from
Aug 17, 2017

Conversation

ro-joowan
Copy link
Contributor

@ro-joowan ro-joowan commented Jul 26, 2017

This PR is mapped to this Swagger change: Azure/azure-rest-api-specs#1429

  • Revised the inheritance structure and broke up objects for customer-experience clarity

  • For more details, please refer to the PR above

  • Found two bugs that I needed to fix in order to run tests in Playback mode:

    • including vcrpy in requirements.txt
    • resource-manager to data-plane in swagger_to_sdk_config.json for catalog.json and job.json
  • There were also a couple modifications that I made to azure-mgmt/tests/test_mgmt_datalake_analytics.py in order for the tests to pass. However, all the tests in the automatic PR passed, which was odd.

* Found two bugs that I needed to fix in order to run tests in Playback
mode:

* including vcrpy in requirements.txt
* resource-manager to data-plane in swagger_to_sdk_config.json for
catalog.json and job.json
@msftclas
Copy link

@ro-joowan,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@begoldsm
Copy link
Contributor

@lmazuel here is the aggregated PR for python that I promised you :). I reviewed this with @ro-joowan and helped him get it done. For some reason the automatic PR passed while this one required some test updates, so I am glad that we did it this way and caught the test failures :).

@codecov-io
Copy link

codecov-io commented Jul 26, 2017

Codecov Report

Merging #1324 into master will increase coverage by 0.04%.
The diff coverage is 84.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1324      +/-   ##
==========================================
+ Coverage   56.14%   56.19%   +0.04%     
==========================================
  Files        2719     2728       +9     
  Lines       71614    71708      +94     
==========================================
+ Hits        40205    40293      +88     
- Misses      31409    31415       +6
Impacted Files Coverage Δ
...ke/analytics/catalog/models/usql_database_paged.py 100% <ø> (ø) ⬆️
...talake/analytics/catalog/models/usql_view_paged.py 100% <ø> (ø) ⬆️
...e/analytics/catalog/models/usql_procedure_paged.py 100% <ø> (ø) ⬆️
...ytics/job/models/job_pipeline_information_paged.py 100% <ø> (ø) ⬆️
...ytics/catalog/models/usql_table_partition_paged.py 100% <ø> (ø) ⬆️
...e/analytics/account/models/sas_token_info_paged.py 80% <ø> (ø) ⬆️
...ake/analytics/catalog/models/usql_package_paged.py 80% <ø> (ø) ⬆️
.../analytics/catalog/models/usql_credential_paged.py 100% <ø> (ø) ⬆️
...ytics/account/models/storage_account_info_paged.py 80% <ø> (ø) ⬆️
...alake/analytics/catalog/models/usql_table_paged.py 100% <ø> (ø) ⬆️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06a5c0d...15431a7. Read the comment docs.

Copy link
Member

@lmazuel lmazuel left a comment

Choose a reason for hiding this comment

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

At a glance:

  • Remove UpgradeLog.htm
  • ChangeLog needs to be more customer focus
  • Remove PTVS files, and files that are specific of your computer

.gitignore Outdated
@@ -4,6 +4,7 @@ __pycache__/

# Virtual environment
env*/
myEnv/
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this :)

@@ -2,6 +2,12 @@

Release History
===============
0.2.0 (2017-07-25)
++++++++++++++++++
* Create an inheritance structure for GET and LIST ADLS accounts.
Copy link
Member

Choose a reason for hiding this comment

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

ChangeLog should be customer focus: what is the actual difference for my code. If you change 90% of the code, but it's changing nothing, I don't want a ChangeLog. Opposite, if you change only a comma, but it's breaking I want why.

Then, the current bullet list is more a tasks list than a ChangeLog :)

requirements.txt Outdated
@@ -2,3 +2,4 @@ futures;python_version<="2.7"
python-dateutil
pyopenssl
msrestazure>=0.4.0,<0.5.0
vcrpy
Copy link
Member

Choose a reason for hiding this comment

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

Remove that, vcrpy is installed by the test requirements file (see .travis.yaml)

azure.pyproj Outdated
@@ -2885,6 +2884,17 @@
<Content Include="setup.cfg" />
<Content Include="swagger_to_sdk_config.json" />
</ItemGroup>
<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

I don't want this file in the PR, this is not supposed to be checked in

* related
* Create a "Basic" jobInformation that is returned for LIST calls
* Setup inheritance for GET jobs
* Create an inheritance structure for GET and LIST ADLA accounts.
Copy link
Member

Choose a reason for hiding this comment

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

See comments on the other ChangeLog

@lmazuel
Copy link
Member

lmazuel commented Aug 3, 2017

Approved based on the comments I made. Up to you to tell me when it's ready to be release :)

Thanks!

@ro-joowan
Copy link
Contributor Author

Please merge and publish!

Thank you,
Joo Wan

@lmazuel lmazuel merged commit 25ec190 into Azure:master Aug 17, 2017
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.

5 participants