Skip to content

Comments

Update Batch SDK to use new API version 2018-12-01#4072

Merged
lmazuel merged 3 commits intoAzure:masterfrom
xingwu1:master
Dec 20, 2018
Merged

Update Batch SDK to use new API version 2018-12-01#4072
lmazuel merged 3 commits intoAzure:masterfrom
xingwu1:master

Conversation

@xingwu1
Copy link
Member

@xingwu1 xingwu1 commented Dec 14, 2018

No description provided.

@adxsdk6
Copy link

adxsdk6 commented Dec 14, 2018

Can one of the admins verify this patch?

1 similar comment
@adxsdk6
Copy link

adxsdk6 commented Dec 14, 2018

Can one of the admins verify this patch?

@xingwu1
Copy link
Member Author

xingwu1 commented Dec 14, 2018

@matthchr @bgklein Please review the change.

Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

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

Had one minor comment on the changelog

Copy link
Member

Choose a reason for hiding this comment

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

Use [ around breaking (match with below)

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 remove the [] to match the old history.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I beleive this description is overwritten by patch.py build_new_add_collection method, so if we want this displayed in documentation must copy there as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@bgklein
Copy link
Contributor

bgklein commented Dec 14, 2018

Did we want to ping/have @lmazuel run his magical script to collapse models to one file. Should make doc's a little cleaner and solve the import issue Jake ran into.

@xingwu1 xingwu1 force-pushed the master branch 4 times, most recently from 68e7d10 to 3592e7b Compare December 14, 2018 22:38
@xingwu1 xingwu1 force-pushed the master branch 3 times, most recently from 8da7efd to bd0a358 Compare December 14, 2018 23:01
@Azure Azure deleted a comment from codecov-io Dec 15, 2018
@lmazuel
Copy link
Member

lmazuel commented Dec 15, 2018

@bgklein I think you should indeed :)

Copy link
Member

Choose a reason for hiding this comment

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

Not ok, keep the pkgutil file

Copy link
Member Author

Choose a reason for hiding this comment

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

Fiexed.

Copy link
Member

Choose a reason for hiding this comment

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

@xingwu1 You need to remove line 1, right now your file has two lines, one with pkgresources, one with pkgutil

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@xingwu1
Copy link
Member Author

xingwu1 commented Dec 17, 2018

@lmazuel Do you have the script? How can I run it?

@lmazuel
Copy link
Member

lmazuel commented Dec 17, 2018

@xingwu1 Script is here:
https://gist.github.com/lmazuel/dad0ac92c733b27f6d81ea57a47625ce#file-patch_models_v2-py

You need to pip install -e azure-batch, and call the script "python patch_models_v2.py azure.batch". This should patch locally your files, to something that looks like this:
https://github.com/Azure/azure-sdk-for-python/tree/master/azure-cognitiveservices-vision-contentmoderator/azure/cognitiveservices/vision/contentmoderator/models

@iscai-msft is currently porting the logic of this script into Autorest itself, for Autorest.Python v4. But the output will be exactly the same. This script is used already by the CLI to shrink the Windows MSI installer (for a few months), so it's safe to assume it's stable.

@xingwu1
Copy link
Member Author

xingwu1 commented Dec 17, 2018

@lmazuel Let me know if there has anything to change.

@codecov-io
Copy link

codecov-io commented Dec 18, 2018

Codecov Report

Merging #4072 into master will decrease coverage by 0.07%.
The diff coverage is 99.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4072      +/-   ##
==========================================
- Coverage   54.23%   54.16%   -0.08%     
==========================================
  Files       10241     9830     -411     
  Lines      207887   207475     -412     
==========================================
- Hits       112751   112373     -378     
+ Misses      95136    95102      -34
Impacted Files Coverage Δ
azure-batch/azure/batch/models/_models.py 65.62% <ø> (ø)
azure-batch/azure/batch/custom/patch.py 80.18% <ø> (ø) ⬆️
azure-batch/azure/batch/models/_models_py3.py 65.62% <ø> (ø)
.../azure/batch/models/_batch_service_client_enums.py 100% <100%> (ø)
azure-batch/azure/batch/models/_paged_models.py 100% <100%> (ø)
azure-batch/azure/batch/models/__init__.py 100% <100%> (ø) ⬆️
azure-batch/azure/batch/batch_service_client.py 95.55% <83.33%> (+0.2%) ⬆️
...re-batch/azure/batch/operations/task_operations.py 73.82% <0%> (ø) ⬆️
...re-batch/azure/batch/operations/file_operations.py 74.68% <0%> (ø) ⬆️
.../azure/batch/operations/compute_node_operations.py 71.91% <0%> (ø) ⬆️
... and 132 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 26611f2...20bd704. Read the comment docs.

@lmazuel
Copy link
Member

lmazuel commented Dec 18, 2018

@xingwu1 @matthchr @bgklein LGTM, I feel ready to merge and release once I have your go :)

@bgklein
Copy link
Contributor

bgklein commented Dec 18, 2018

Changes look good to me. @matthchr is OOF until the new year. We should be good to merge and release

@lmazuel
Copy link
Member

lmazuel commented Dec 19, 2018

@bgklein "should" ? :). May I humbly ask you a more assertive sentence about this, since once it's released, it cannot be undone :)

@xingwu1
Copy link
Member Author

xingwu1 commented Dec 19, 2018

@lmazuel please merge it.

@lmazuel lmazuel merged commit 92c969f into Azure:master Dec 20, 2018
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.

6 participants