-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix Nic creation issue in stack #4008
Conversation
@viananth, |
This change needs tests IMHO as that's likely why it wasn't caught in the first place. |
nic = NetworkInterface(location=location, tags=tags, enable_ip_forwarding=enable_ip_forwarding, | ||
dns_settings=dns_settings, enable_accelerated_networking=enable_accelerated_networking) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be more concise and consistent with the rest of the CLI patterns:
nic = NetworkInterface(location=location, tags=tags, enable_ip_forwarding=enable_ip_forwarding, dns_settings=dns_settings)
if supported_api_version(ResourceType.MGMT_NETWORK, min_api='2016-09-01'):
nic.enable_accelerated_networking = enable_accelerated_networking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be more concise and consistent with the rest of the CLI patterns:
nic = NetworkInterface(location=location, tags=tags, enable_ip_forwarding=enable_ip_forwarding, dns_settings=dns_settings)
if supported_api_version(ResourceType.MGMT_NETWORK, min_api='2016-09-01'):
nic.enable_accelerated_networking = enable_accelerated_networking
Codecov Report
@@ Coverage Diff @@
## master #4008 +/- ##
==========================================
- Coverage 70.59% 70.51% -0.09%
==========================================
Files 427 428 +1
Lines 27541 27561 +20
Branches 4213 4219 +6
==========================================
- Hits 19443 19434 -9
- Misses 6837 6873 +36
+ Partials 1261 1254 -7
Continue to review full report at Codecov.
|
@derekbekoe enabling multi-cloud testing is something for us to do, but yes, now that stack has been merged we are overdue for it. @troydai where is that on the roadmap? |
Also, the existing NIC test would have caught the regression if it were run on the stack profile since it didn't have any API restrictions. However, right now it is just reliant upon us remembering to manually test on the other profile which is not a sustainable plan moving forward. For @viananth, this does mean that you should also add the appropriate version constraint to the NetworkNicScenarioTest in the network tests. |
@tjprescott multi-cloud testing should already be there but @troydai can confirm. |
No plan for multi-cloud testing for now. But I'm all ears. |
@tjprescott @derekbekoe I'm working on the creating tests for stack cloud. But meanwhile, could we please merge this PR as this is breaking our customers? |
Nic creation in Azure Stack failed because network api supported by stack doesn't recognize
'enable_accelerated_networking'
argument for nic creation. This fix adds a check for supported api version.Closes #4007
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)