Changes related to fixing the inheritance logic. #1009
Conversation
vishrutshah
left a comment
vishrutshah
left a comment
There was a problem hiding this comment.
We need to release the generator and then update/regen all gems to make this work right?
| ip_address_path = ni_instance.ip_configurations[0].public_ipaddress.id | ||
| ip_address = network_client.make_request(:get, ip_address_path, options) | ||
| ip_address_instance = PublicIPAddress.new.from_json(ip_address) | ||
| ip_address_instance = network_client.deserialize(PublicIPAddress.mapper(), ip_address) |
There was a problem hiding this comment.
just curious what has caused this change?
There was a problem hiding this comment.
I was working with David on a different task. It was dropped eventually. This was a remnant from that task
| namespace_parameters = Azure::ARM::ServiceBus::Models::SBNamespace.new | ||
| namespace_parameters.location = 'westus' | ||
| queue_parameters = Azure::ARM::ServiceBus::Models::SBQueue.new | ||
| queue_parameters.location = 'westus' |
There was a problem hiding this comment.
any reason for removing this? Just wanted to make sure what has caused this?
There was a problem hiding this comment.
Because accroding to the Resource definition in ServiceBus queue, there is no location. https://github.com/Azure/azure-rest-api-specs/blob/current/specification/servicebus/resource-manager/Microsoft.ServiceBus/2017-04-01/servicebus.json#L2503
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
Agreeing with Vishrut, these changes should be together with generation of the SDK based on the new generator, we shouldn't merge them yet.
Also, why is CI passing for this PR? shouldn't the test case update cause it to fail? or is it because of the manual edit of the files in the last release?
|
@veronicagg The CI will pass as it is running against the current code. |
|
@vishrutshah Changed the expect per offline discussion |
| result = @resource_helper.network_client.network_interfaces.list_all | ||
| result.each do |network_interface| | ||
| expect(network_interface.virtual_machine).to be_an_instance_of(MsRestAzure::SubResource) | ||
| expect(network_interface.virtual_machine).to be_an_instance_of(VirtualMachine) |
There was a problem hiding this comment.
you should have to write Azure::ARM::Compute::Models::VirtualMachine
There was a problem hiding this comment.
Actually, it should be Azure::ARM::Network::Models::SubResource. The model looks like this:
class NetworkInterface < Resource
include MsRestAzure
# @return [SubResource] The reference of a virtual machine.
attr_accessor :virtual_machine
So, changed it with the latest commit
sarangan12
changed the title
sarangan12
changed the title
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
LGTM!
Minor question inline.
For the test, line #1009 (diff) you may want to add a comment with a pointer to the spec where it indicates that "virtualmachine" is defined as "Subresource", just so it's clear in the future on why we're checking against that.
|
|
||
| include MsRestAzure | ||
|
|
||
| include MsRest::JSONable |
There was a problem hiding this comment.
why was this included before?
There was a problem hiding this comment.
It was a remnant from a serialization task which was later dropped.
|
@veronicagg Added the link in the comment. |
3 participants
As discussed offline, this PR is related to Azure/autorest.ruby#13 and fixes the tests associated with those changes.
@vishrutshah @veronicagg @salameer Please review and approve