Updated to v7.0.1 and code looks better now #3
Conversation
|
LGTM |
| 4. Get this sample using command `go get -u github.com/Azure-Samples/resource-manager-go-resources-and-groups`. | ||
| 5. Get the [Azure SDK for Go](https://github.com/Azure/azure-sdk-for-go) using command `go get -u github.com/Azure/azure-sdk-for-go`. Or in case that you want to vendor your dependencies using [glide](https://github.com/Masterminds/glide), navigate to this sample's directory and use command `glide install`. | ||
| 6. Compile and run the sample. | ||
| 1. If you don't already have it, [install Go 1.7](https://golang.org/dl/). |
There was a problem hiding this comment.
I'd remove the version number here, I don't think there's anything in particular tying us to Go v1.7 instead of Go v1.6 or Go v1.8
There was a problem hiding this comment.
The SDK does not work with anything below 1.7
History time :D
|
|
||
| The sample gets an authorization token, creates a new resource group, updates it, lists the resource groups, creates a resource using a generic template, updates the resource, lists the resources inside the resource group, exports all resources into a json template, deletes a resource, and deletes the resource group. | ||
| ``` | ||
| git clone https://github.com:Azure-Samples/virtual-machines-go-manage.git |
There was a problem hiding this comment.
Thoughts on offering go get instructions as an option here as well as cloning?
| onErrorFail(err, "ListResources failed") | ||
| if resourcesList.Value != nil && len(*resourcesList.Value) > 0 { | ||
| fmt.Printf("Resources in '%s' resource group\n", groupName) | ||
| for _, resource := range *resourcesList.Value { |
There was a problem hiding this comment.
We should probably update this function to accommodate Resource groups that contain multiple pages of results. i.e. NextLink isn't nil
| value := os.Getenv(varName) | ||
| if value == "" { | ||
| fmt.Printf("Missing environment variable %s\n", varName) | ||
| os.Exit(1) |
There was a problem hiding this comment.
In general I'm not a fan of os.Exit. If it doesn't add too much overhead, I'd try to use returned values to decide whether or not to bail. It'd allow us to clean up resources created for the example a la defer.
There was a problem hiding this comment.
This is something that I took from @jhendrixMSFT sample.It would be better if we three agree on how we handle this (and all samples have the same style).
There was a problem hiding this comment.
Ugh I didn't realize that os.Exit() skips deferred functions. :( The two things I wanted to achieve here are to centralize (as much as possible) termination-on-failure behavior and ensure a non-zero exit code on failure. Originally I was using panic() however that doesn't appear to be "idiomatic go". We can come up with another pattern or perhaps using panic() is good enough for the samples.
There was a problem hiding this comment.
I think panic is good enough for samples.
Still there are some errors that if they happen, the sample simply cannot continue (here, without the env vars for the token, there is not a lot we can do...). But there are other errors that it doesnt really matter if they happen, because the sample can continue (here, list resource groups).
For onErrorFail, we can also delete the complete resource group, but sometimes it is good to still have the resources and look at then in the portal to understand what went wrong.
There was a problem hiding this comment.
Oh, and if we want to use the samples in CI, it is better to break them, hohoho (and always delete all resources or other samples would be affected)
| func onErrorFail(err error, message string) { | ||
| if err != nil { | ||
| fmt.Printf("%s: %s\n", message, err) | ||
| os.Exit(1) |
PTAL :D
@jhendrixMSFT @marstr