Alpha of ARM packages #185

brendandixon · 2015-08-13T17:35:45Z

Alpha of generated ARM packages

Signed-off-by: Brendan Dixon brendand@microsoft.com

Signed-off-by: Brendan Dixon <brendand@microsoft.com>

msftclas · 2015-08-13T17:35:49Z

Hi @brendandixon, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

brendandixon · 2015-08-13T17:36:11Z

@ahmetalpbalkan @paulmey @devigned Ok folks. Here's an alpha of the ARM packages.

msftclas · 2015-08-13T17:44:36Z

@brendandixon, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

ahmetb · 2015-08-13T18:28:30Z

arm/storage/client.go

+
+func NewWithBaseUri(baseUri string, subscriptionId string) *StorageManagementClient {
+	client := &StorageManagementClient{BaseUri: baseUri, SubscriptionId: subscriptionId}
+	client.PollingMode = autorest.PollUntilDuration


area for little idiomatic go improvement: these can probably go into the struct initializer above.

Not sure I follow. These routines are called by other routines, some of which may provide new values.

I mean you can replace:

client := &StorageManagementClient{BaseUri: baseUri, SubscriptionId: subscriptionId} client.PollingMode = autorest.PollUntilDuration

with the following:

return &StorageManagementClient{ BaseUri: baseUri, SubscriptionId: subscriptionId, Client: autorest.Client{ PollingMode: autorest.PollUntilDuration, }}

so you don't mix “assignments” with struct “initializers” and you can just return this single statement..

@ahmetalpbalkan Got it. The pronoun above w/o a referent left the question in my mind.

ahmetb · 2015-08-13T18:40:46Z

Here's my first main comment: I see this pattern in pretty much a lot of packages.

DefaultBaseUri = "https://management.azure.com"
NewXXXClientWithBaseUri(subscriptionId string)
NewXXXClientWithBaseUri(baseUri string, subscriptionId string)

in pretty much all packages. I think this is

creates duplication across packages.
limits us from changing the method signatures. (will describe below)
limits the amount of things user can configure. for instance, baseUri is configurable by the user, but polling duration isn't. How about we add a retry logic in the future? How about if the client wants to configure an User-Agent etc?

If I'm calling into 5 different packages, I'm initializing 5 different clients by passing my subscription ID 5 times. This doesn't make sense to me:

sc := storage.New(MY_SUBS_ID)
// sc.doThis();
net := network.New(MY_SUBS_ID)
// net.doThat();
co := compute.New(MY_SUBS_ID)
// co.doThat();

I think a better model (and what we follow in the management/... package) would be:

azClient := azure.New(MY_SUBS_ID, AND_BUNCH_OF_OTHER_THINGS
                         LIKE_RETRY_POLICY,USER-AGENT,POLLING_INTERVAL_WHATEVER)

net := network.New(azClient)
stg := storage.New(azClient)
cmp := compute.New(azClient)

Here's what I just achieved with this:

All clients initialized exactly the same.
I can freely more config like baseUrl, pollingInterval, userAgent, retryPolicy whatever to the azClient and I don't even change individual packages == I don't break a lot of places when I need to change something.
It's more readable.
I don't pass subscription ID to 5 different places.
I don't duplicate fields in type FooClient struct { autorest.Client , SubscriptionId string, BaseUri string } etc. in every single client. My types just look like type FooClient struct { c azure.Client }.
I don't duplicate DefaultBaseUri, DefaultPollingDuration etc in every single client package.

It would be great if we can fix it this way. @brendandixon if you also agree, I'd like to take this fix because it will be breaking change.

ahmetb · 2015-08-13T18:44:21Z

[followup to the comment above]

We have this model already in management/... package. Please take a look:

azure-sdk-for-go/management/client.go

Lines 70 to 104 in 99b5c36

    
           type ClientConfig struct { 
        
           	ManagementURL         string 
        
           	OperationPollInterval time.Duration 
        
           	UserAgent             string 
        
           	APIVersion            string 
        
           } 
        
           // NewAnonymousClient creates a new azure.Client with no credentials set. 
        
           func NewAnonymousClient() Client { 
        
           	return client{} 
        
           } 
        
           // DefaultConfig returns the default client configuration used to construct 
        
           // a client. This value can be used to make modifications on the default API 
        
           // configuration. 
        
           func DefaultConfig() ClientConfig { 
        
           	return ClientConfig{ 
        
           		ManagementURL:         DefaultAzureManagementURL, 
        
           		OperationPollInterval: DefaultOperationPollInterval, 
        
           		APIVersion:            DefaultAPIVersion, 
        
           		UserAgent:             DefaultUserAgent, 
        
           	} 
        
           } 
        
           // NewClient creates a new Client using the given subscription ID and 
        
           // management certificate. 
        
           func NewClient(subscriptionID string, managementCert []byte) (Client, error) { 
        
           	return NewClientFromConfig(subscriptionID, managementCert, DefaultConfig()) 
        
           } 
        
           // NewClientFromConfig creates a new Client using a given ClientConfig. 
        
           func NewClientFromConfig(subscriptionID string, managementCert []byte, config ClientConfig) (Client, error) { 
        
           	return makeClient(subscriptionID, managementCert, config) 
        
           }

azure-sdk-for-go/management/storageservice/client.go

Lines 24 to 26 in 99b5c36

    
           func NewClient(s management.Client) StorageServiceClient { 
        
           	return StorageServiceClient{client: s} 
        
           }

azure-sdk-for-go/management/storageservice/entities.go

Lines 10 to 12 in 99b5c36

type StorageServiceClient struct {

client management.Client

}

You can see how tiny the individual FooClient types in each package are.

devigned · 2015-08-13T18:49:01Z

/cc @Azure/adx-sdk-team

brendandixon · 2015-08-13T19:03:24Z

@ahmetalpbalkan We should chat. First, the generator must handle more than just ARM clients. It does not know that these packages are related and, hence, cannot divine the need for a common "base" class. Second, the go-autorest package provides a "base" class used by all generated classes (autorest.Client) which provides public access to everything and more (e.g. client.PollingDuration, client.PollingMode). It also provides easy-to-use "hooks" by which to provide a modified version of http.Client and more. So, the user has full access to everything. The rest is somewhat imposed on the code by the generator model.

brendandixon · 2015-08-13T19:05:37Z

@ahmetalpbalkan By the way, in my experience, it is generally a poor practice to expose, as individual arguments, things a user may want to configure. It tends to be much too fragile. Better models, and that which I've tried to provide through the go-autorest package, allows users to make modifications w/o the core code having to know all things.

ahmetb · 2015-08-13T19:15:17Z

@brendandixon fair enough. If the code generation model enforces this model, we can live with it. I guess it would be nice to have all the client configurations centralized in one place because pretty much nobody just initializes only one client, it's nearly always a mix of those.

I didn't realize all those fields were public, so it's all good as long as it is configurable.

brendandixon · 2015-08-13T20:15:38Z

@ahmetalpbalkan It actually supports configuration well-beyond those few values. Users may provide instances of http.Client that do whatever magic they desire (e.g., handle proxies, implement circuit breakers, log requests / responses). Further the model supports fan-in / out through channels w/o the developer having to writes lots of code.

I could add an initializer / constructor that accepts, along with other values, an instantiated autorest.Client. Further reduction of repetition, given how the generator must work, is best handled via convention (e.g., using a local, well-defined Azure properties file -- which we really need to have and do not (yet) have). Such an approach will never break signatures and reduces the risk of checking in bad things (e.g., keys) to central repositories.

@ahmetalpbalkan I think I've come up with a means by which the user can, if they desire, create one autorest.Client to share. I'll need to think through the cases, but I believe it will work. If it does, then I can eliminate some of the seemingly redundant code.

ahmetb · 2015-08-13T20:30:06Z

arm/authorization/client.go

+	return client
+}
+
+////////////////////////////////////////////////////////////////////////////////


Currently this block looks like a godoc (because it's just above the type declaration). Maybe have a new line after this comment block?

@ahmetalpbalkan I'm sorry, but which line? The type declaration occurs before the two methods. Are the comment-less methods a problem? I tended to direct the comments to the block before the type.

Here's what godoc you're currently generating here looks like :

Either have an empty line after this code block or have proper Godoc for the type. :)

@ahmetalpbalkan Got it. Thank you. I failed to see that. The files are long and need some kind of divider. I failed to see that these collide with good. I'll fix.

@brendandixon if they're indeed different clients and you're having trouble skimming long files why not split different client types into different source files such as:

compute/availabilitysets.go

compute/availabilitysets_entities.go

compute/virtualmachineimages.go

compute/virtualmachineimages_entities.go

@ahmetalpbalkan Definitely an option and easy to do.

@brendandixon that would be really nice to have IMO.

@ahmetalpbalkan Will do. I will keep all types in a single, models.go, file, however.

ahmetb · 2015-08-13T22:11:38Z

arm/compute/client.go

+}
+
+// ListPublishers gets a list of virtual machine image publishers.;
+//


no biggie: Looks like when the method has no parameters to document, the godoc is followed by an extra empty comment like:

// MethodName description lorem ipsum. // func (....

ahmetb · 2015-08-13T22:29:41Z

arm/compute/client.go

+		return result, fmt.Errorf("compute: Failure sending VirtualMachineExtensionImagesClient.ListVersions request (%v)", err)
+	}
+
+	result = []VirtualMachineImageResource{}


I think there's a bit of difference between

result = []VirtualMachineImageResource{}

and

var result []VirtualMachineImageResource

. The first one actually initializes something that takes up space in memory (i.e. zero-length slice) whereas the second one is just nil-slice. It will not cause any problems here but just to be idiomatic, I'd do var foo []T.

@ahmetalpbalkan The code is correct as it is. json.Unmarshal requires a pointer to an existing struct.

@brendandixon I'm not sure what you're talking about. Both expressions I listed above are of the same types, and both will work with json.Unmarshal as is. Check this example out: http://play.golang.org/p/P0yLMrZYfN

@ahmetalpbalkan And they're the same size. See http://play.golang.org/p/Rho7YDbGDE.

Leaving it as it is leaves the generator less complicated (otherwise we have to add a case to handle this...).

@ahmetalpbalkan I was actually surprised that json.Unmarshal accepted []Foo form. Now that I see both are the same size, it is apparent that []Foo is equivalent to []Foo{}.

@brendandixon umm, equivalent, not really. :) If you actually compare the two forms using == you'll get a false. One does an allocation in stack, the other one doesn't.

Totally off topic but here's a benchmark: http://play.golang.org/p/XBLXdWv9_8 the one that doesn't initialize the empty slice is ~20% faster. Again, doesn't really matter in this case. It's more of a styling argument here.

✅

ahmetb · 2015-08-17T22:07:03Z

@brendandixon generally LGTM, feel free to merge at your own discretion.

Alpha of ARM packages

68fb142

Signed-off-by: Brendan Dixon <brendand@microsoft.com>

msftclas added the cla-required label Aug 13, 2015

msftclas added cla-signed and removed cla-required labels Aug 13, 2015

ahmetb reviewed Aug 13, 2015
View reviewed changes

ahmetb mentioned this pull request Aug 13, 2015

Add Azure Resource Manager support #114

Closed

ahmetb reviewed Aug 13, 2015
View reviewed changes

brendandixon closed this Aug 17, 2015

richardpark-msft pushed a commit to richardpark-msft/azure-sdk-for-go that referenced this pull request Aug 5, 2021

fix test reporting for integration pipeline (Azure#185)

9762389

benbp pushed a commit to benbp/azure-sdk-for-go that referenced this pull request Sep 15, 2021

fix test reporting for integration pipeline (Azure#185)

907f574

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alpha of ARM packages #185

Alpha of ARM packages #185

brendandixon commented Aug 13, 2015

msftclas commented Aug 13, 2015

brendandixon commented Aug 13, 2015

msftclas commented Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb commented Aug 13, 2015

ahmetb commented Aug 13, 2015

devigned commented Aug 13, 2015

brendandixon commented Aug 13, 2015

brendandixon commented Aug 13, 2015

ahmetb commented Aug 13, 2015

brendandixon commented Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 13, 2015

brendandixon Aug 13, 2015

brendandixon Aug 13, 2015

ahmetb Aug 14, 2015

ahmetb commented Aug 17, 2015

Alpha of ARM packages #185

Alpha of ARM packages #185

Conversation

brendandixon commented Aug 13, 2015

msftclas commented Aug 13, 2015

brendandixon commented Aug 13, 2015

msftclas commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Aug 13, 2015

ahmetb commented Aug 13, 2015

devigned commented Aug 13, 2015

brendandixon commented Aug 13, 2015

brendandixon commented Aug 13, 2015

ahmetb commented Aug 13, 2015

brendandixon commented Aug 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Aug 17, 2015