-
Notifications
You must be signed in to change notification settings - Fork 825
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
Client refactor #48
Client refactor #48
Conversation
Better commit message maybe? |
//NewAnonymouseClient creates a new azure.Client with no credentials set. | ||
// ClientConfig provides a configuration for use by a Client | ||
type ClientConfig struct { | ||
ManagementCert []byte |
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.
since you switched PRs (something we shouldn't do 😄) you lost my comment here. this ManagementCert is not a client configuration. It's regarding how the client is fundamentally built.
Assume you're adding OAuth support and now are you gonna put a token string
right next to this? That would be weird... When something like this happens it just means there's a design mistake, in this case you're putting unrelated things together. Config should be things that are shared regardless of how the client is built. Some examples: managementURL
, useHTTPS
, logger
, timeout
, retryPolicy
etc...
So you should probably replace the NewClientFromConfig to take managementCert []byte
parameter along with config
.
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.
I agree. Good points. Will refactor this.
@ahmetalpbalkan this is driving me insane. So I cloned the MSOpenTech repo in to my Go workspace and added my fork as a separate remote. What should my exact steps be to update my feature branch off my fork to ensure that when I make subsequent commits on this feature branch it doesn't include commits that are already part of the master branch? The steps that I'm using now are as follows:
|
|
Thanks @ahmetalpbalkan so those steps I have down...but how do I update my multi-env branch with the latest revisions in origin/master? |
you shouldn't be working on your master. all your changes must be on a branch named
|
Signed-off-by: Andrew Weiss <[email protected]>
Thanks! Got it all fixed and now know the process for future PRs. |
// NewClient creates a new Client using the given subscription ID and | ||
// management certificate | ||
func NewClient(subscriptionID string, managementCert []byte) (Client, error) { | ||
return makeClient(subscriptionID, managementCert, defaultAzureManagementURL) |
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 really call NewClientFromConfig
with "default config" (in this case we just have the URL but in the future, default retry policy, etc).
Signed-off-by: Andrew Weiss <[email protected]>
LGTM, will merge. |
Support custom configuration and `[]byte` cert constructor
* Complete parameters for table operations * Complete parameters in entity operations * Complete parameters in query table operation * Added get entity operation * Added get table operation * Included test recordings * Fixed merge little mess * Fix travis
* Complete parameters for table operations * Complete parameters in entity operations * Complete parameters in query table operation * Added get entity operation * Added get table operation * Included test recordings * Fixed merge little mess * Fix travis
* Complete parameters for table operations * Complete parameters in entity operations * Complete parameters in query table operation * Added get entity operation * Added get table operation * Included test recordings * Fixed merge little mess * Fix travis
Refactor client for consolidation and to support custom config options