-
Notifications
You must be signed in to change notification settings - Fork 860
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
Add a client for Virtual Network management #33
Conversation
return err | ||
} | ||
|
||
fmt.Println("Content", string(networkConfigurationBytes)) |
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.
Do you need this?
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.
Nope, that was an oversight from when I was trying to debug the schema. I'll remove.
Yeah API doesn't support concurrency. :) It's easy to run into race conditions in VNet REST APIs. Feel free to upvote my feedback: http://feedback.azure.com/forums/217313-networking-dns-traffic-manager-vpn-vnet/suggestions/5954992-make-virtual-network-operations-atomic-on-portal-a |
//configuration for the currently active subscription. Note that the | ||
//underlying Azure API means that network related operations are not safe | ||
//for running concurrently. | ||
func GetVirtualNetworkConfiguration() (*NetworkConfiguration, error) { |
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.
shouldn't it be just NetworkConfiguration, without pointer?
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.
Can see arguments for either here. Will look through what other clients do and potentially change to returning the value instead.
@ahmetalpbalkan re: concurrency a reasonable way to do this would be to support |
//currently active subscription according to the NetworkConfiguration given. | ||
//Note that the underlying Azure API means that network related operations | ||
//are not safe for running concurrently. | ||
func SetVirtualNetworkConfiguration(networkConfiguration *NetworkConfiguration) error { |
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.
Check for null param here?
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 should not be a pointer anyway.
Can you fix Travis build please? |
rebasing the commit on top of master should be fixing the problem. |
fcc5302
to
2632d1a
Compare
Yes, rebasing and changing the use of unexported symbols fixes this. |
By the way, what's the thought in this project of squashing commits prior to PR? Prefer the clean history or the complete change sets? |
For the changes essentially changing one major thing, one squashed commit preferable. Thanks. |
Do you want me to squash this one? |
Yes please. |
This commit adds entities to generate the (unavailable) XML schema for virtual network specification, and a client with Get and Set operations for the currently active subscription. The pattern for using the client is the same as the powershell version - get the running config, merge in any desired changes and set the configuration to the new state. There is no documentation on what the concurrency constraints are with the underlying Azure REST API, I suspect it is not safe to call concurrently though can't verify this.
2632d1a
to
c95e423
Compare
LGTM |
👍 in use it's actually a bit derpy because of the semantics around modifying the "document" that gets returned when it's not a pointer. May revisit that in future. |
Add a client for Virtual Network management
* Including date and api version in request error * Date as a string
* Including date and api version in request error * Date as a string
* Including date and api version in request error * Date as a string
make all examples and remove extra ctxs
This pull requests support for managing virtual networks in Azure.
It adds structs to parse and generate the (seemingly unavailable) XML schema for virtual network specification, and a client with Get and Set operations for the currently active subscription.
The pattern for using the client is the same as the powershell version - get the running config, merge in any desired changes and set the configuration to the new state. There is no documentation on what the concurrency constraints are with the underlying Azure REST API, I suspect it is not safe to call concurrently though can't verify this.
It was also necessary to modify the HTTP client to accept different content types, since the underlying REST API requires the PUT operation to be made with a content type of
text/plain
, despite it actually being XML.