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

[Ready for review] Graph & Key Vault MGMT #1008

Merged
merged 33 commits into from
Sep 13, 2016

Conversation

jianghaolu
Copy link
Contributor

No description provided.

@azurecla
Copy link

azurecla commented Aug 3, 2016

Hi @jianghaolu, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). 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.azure.com.

TTYL, AZPRBOT;

@azuresdkci
Copy link
Contributor

Runtime changes detected. pull request created. CI running: Build Status

@jianghaolu jianghaolu force-pushed the graphkeyvault branch 2 times, most recently from 571391e to d36d5a2 Compare August 10, 2016 23:19
@jianghaolu jianghaolu changed the title [InProgress] Graph & Key Vault MGMT [Ready for review] Graph & Key Vault MGMT Sep 9, 2016
@jianghaolu
Copy link
Contributor Author

@martinsawicki
This will show a diff without generated files: 0484ce6...93be0f4

@martinsawicki
Copy link

hmm, that view doesn't let me add comments..

@martinsawicki
Copy link

In general, from my quick look, the following 3 key things caught my eye:

  • What is called "Group" should probably better be called "ActiveDirectoryGroup" (based on the naming of ADGroupInner)
  • Replace Boolean returning methods with boolean
  • Javadoc's @return should start with a small letter and no period
    After the PR, i'll try to play with in in code to see if I'd have more comments.
    I noticed some interesting uses of base interfaces to reuse teh same definition in different contexts - i'll play with that and see if we can reuse taht pattern in other places (does it really avoid raw type refs? It';d be great if it did, without the duplication)

@@ -25,12 +25,12 @@
Updatable<Vault.Update>,
Wrapper<VaultInner> {
/**
* @return The URI of the vault for performing operations on keys and secrets.
* @return the URI of the vault for performing operations on keys and secrets.

Choose a reason for hiding this comment

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

also, no period at the end for @return :-)

@jianghaolu
Copy link
Contributor Author

jianghaolu commented Sep 13, 2016

Thanks! Good catches. I'll fix them.

Two things to notice:

  1. The Azure.authenticate() signature is changed
  2. Graph is in fact in progress. E.g.: Group doesn't have an impl yet. I'll add them in the Graph PR.

@martinsawicki martinsawicki merged commit 8424341 into Azure:master Sep 13, 2016
sima-zhu pushed a commit to sima-zhu/azure-sdk-for-java that referenced this pull request Mar 21, 2019
[Automatic PR] SDK changes from pull request Azure#1008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants