Skip to content
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

Bump to the right version. Switch test account to team account. #4710

Merged
merged 8 commits into from
Jul 31, 2019

Conversation

sima-zhu
Copy link
Contributor

Most of the files are playback json files. Please skip when necessary.

@alzimmermsft alzimmermsft added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jul 30, 2019
this.sharedKeyCredential = SharedKeyCredential.fromConnectionString(connectionString);
getEndPointFromConnectionString(connectionString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see any javadoc changes
Should they be updated in the builder class that endpoint is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to state optional as it is optional only if connectionString built in. If we only build in credential, then endpoint is still necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, so are these scenarios specified in javadocs?
It will be helpful for the user I feel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are descriptions in builder class. I can add some additional doc to make it more clear.

@@ -74,9 +74,7 @@ public AzureFileStorageImpl build() {
this.pipeline = RestProxy.createDefaultPipeline();
}
AzureFileStorageImpl client = new AzureFileStorageImpl(pipeline);
if (this.version != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the version always get set from a static conig class ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is auto-gen class. I have the temp fix to pass the spotbugs. Loop in @jianghaolu for further discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's track this, so it doesn't cause issues later.

pom.client.xml Outdated
@@ -546,6 +546,7 @@
-snippetpath ${project.basedir}/sdk/keyvault/azure-keyvault-keys/src/samples/java
-snippetpath ${project.basedir}/sdk/keyvault/azure-keyvault-secrets/src/samples/java
<!-- -snippetpath ${project.basedir}/storage/client/blob/src/samples/java-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be uncommented?

Copy link
Contributor Author

@sima-zhu sima-zhu Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alzimmermsft can we add back this line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry about uncommenting this line in this PR, blobs doesn't have any snippets yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will uncommon this.

return this;
}

private void getEndPointFromConnectionString(String connectionString) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming this get.. and returning void is misleading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, consider turning this into a utility method as parsing the connection string to get key-value pairs may be used in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will move to common utility after we extract common module. It will address all these pain.
I am doing this just to minimize the the PR's scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer this one: #4658

return this;
}

private void getEndPointFromConnectionString(String connectionString) {
HashMap<String, String> connectionStringPieces = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Map<String, String> connectionStringPieces. Code to the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

return this;
}

private void getEndPointFromConnectionString(String connectionString) {
HashMap<String, String> connectionStringPieces = new HashMap<>();
for (String connectionStringPiece : connectionString.split(";")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you just need the account name, do you need to look at all the connection string key-value pairs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} catch (MalformedURLException e) {
LOGGER.error("There is no valid account for the connection string. "
+ "Connection String: %s", connectionString);
throw new IllegalArgumentException(String.format("There is no valid account for the connection string. "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new guideline is to use logger.logAndThrow() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will address in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sima-zhu sima-zhu requested review from srnagar and g2vinay July 30, 2019 22:50
@sima-zhu sima-zhu merged commit 20a085f into Azure:master Jul 31, 2019
@sima-zhu sima-zhu deleted the storage_tests branch July 31, 2019 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants