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

[Network] Fix Network Bugs 1239, 1240 #1274

Merged
merged 3 commits into from
Nov 9, 2016
Merged

[Network] Fix Network Bugs 1239, 1240 #1274

merged 3 commits into from
Nov 9, 2016

Conversation

tjprescott
Copy link
Member

@tjprescott tjprescott commented Nov 9, 2016

Fixes #1239, fixes #1240.

Plan to defer merging this until #1059 is merged.

@tjprescott tjprescott changed the title Fix Network Bugs 1239, 1240 [Network] Fix Network Bugs 1239, 1240 Nov 9, 2016
Copy link
Contributor

@yugangw-msft yugangw-msft left a comment

Choose a reason for hiding this comment

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

Left one comment to clarify

@@ -63,7 +63,9 @@ def delete_func(resource_group_name, resource_name, item_name):
keep_items = \
[x for x in item.__getattribute__(prop) if x.name.lower() != item_name.lower()]
_set_param(item, prop, keep_items)
return client.create_or_update(resource_group_name, resource_name, item)
result = client.create_or_update(resource_group_name, resource_name, item).result()
if next((x for x in getattr(result, prop) if x.name == item_name), None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider whether the item_name could have casing difference. If yes, do case insensitive comparison

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch!

@tjprescott
Copy link
Member Author

tjprescott commented Nov 9, 2016

@yugangw-msft I made the case insensitive comparison and fixed up the VS project file for the perf change PR. I plan to merge once CI finishes.

@yugangw-msft
Copy link
Contributor

:shipit:

@tjprescott tjprescott merged commit d8ef78b into Azure:master Nov 9, 2016
@tjprescott tjprescott deleted the NetworkBugs branch November 9, 2016 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants