Multi environment support #47
Conversation
| @@ -36,8 +37,14 @@ func NewAnonymousClient() Client { | |||
|
|
|||
| //NewClientFromPublishSettingsFile creates a new azure.Client and imports the publish | |||
| //settings from the specified file path. | |||
| func NewClientFromPublishSettingsFile(publishSettingsFilePath string) (Client, error) { | |||
| client := Client{} | |||
| func NewClientFromPublishSettingsFile(publishSettingsFilePath string, serviceManagementURL ...string) (Client, error) { | |||
|
Please squash changes to 1 commit. I see some intermediate confusing commits that contributes no delta to the PR. |
|
squashed commits, cleaned up base URL declaration. Not sure if this is right approach but learning process for me :) |
| self.client.WaitAsyncOperation(requestId) | ||
|
|
||
| return nil | ||
| return self.client.WaitAsyncOperation(requestId) |
There was a problem hiding this comment.
These are fixed already. Please rebase.
|
not sure how else to consolidate the number of functions here |
| } | ||
|
|
||
| client.baseURL = baseURL | ||
| err := client.importPublishSettings(subscriptionID, managementCertificatePath) |
There was a problem hiding this comment.
You can directly return this line
|
fixed the rebase issue (hunk kept getting staged for some reason)...modified return statements and updated constant to more friendly name. slowly getting the hang of these conventions :) |
| err := client.importPublishSettingsFile(publishSettingsFilePath) | ||
| if err != nil { | ||
| return client, err | ||
| } | ||
| return client, nil | ||
|
|
||
| return Client{ |
There was a problem hiding this comment.
this is particularly bad because if somebody sets client.XXX = YYY, you'll construct a new client :) Also have you noticed that these return Client{baseURL:baseURL},nil snippets are identical? Maybe a new method? 😉
There was a problem hiding this comment.
lol, yep, missed that one...will update that
There was a problem hiding this comment.
it's not entirely bad. I'm also fine with previous version like you set baseURL at the end. but it's probably better to have a newClient(baseURL) method and maybe even return *Client to get rid of being have to declare var client Client...
There was a problem hiding this comment.
So returning a pointer was what I was thinking initially based on other API SDKs I've perused, but didn't want to do too much re-factoring...I guess at this point it wouldn't hurt
There was a problem hiding this comment.
We actually recently decided to not to pass *Client to management libraries because then they can modify it. However if we return *Client here, then we expect people to initialize clients like virtualmachine.NewClient(*client) which is vague because they need to figure out deferencing it with *. It doesn't really sound idiomatic.
|
Can we leave the existing factories unchanged? |
|
We could...although, I feel like the client factories start to become unwieldy as we add more of them to support different parameter sets |
|
@anweiss +1. as we develop this the factory method will just keep getting bigger. it makes sense to at least have one very simple factory and one with multiple parameters or a Config object. |
|
Sorry, I mean the function signatures, not the implementation. |
|
@jeffmendoza yep I'm aware. But we already broke everything. I thought it would be fine. Any specific reasons to not to break this? Maybe we can keep NewClient same and add NewClientFromConfig. |
|
I'm not sure I like the way the publishsettings file/management certs are imported either...which impacts the design of these clients. Currently, the |
|
@anweiss +1 and actually we shouldn't ask for |
|
Ah ok, makes sense. I think keeping the "first found" publishsettings file options would be good for some cases, but also specifying which sub to use would be a great addition. Agree with taking in a byte array for these file. |
|
|
|
Going to close and re-open this PR to clean things up |
|
@anweiss I hate when people do that unless absolutely necessary :-) |
|
lol, I screwed up my rebase with the merged commits from the package refactor...so my commits in this PR kept including changes that were already merged in to master...hence the new branch |
Adds support for declaring a custom service management endpoint. Referencing #40