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

[Storage] add new stroage account kind "FileStorage" and "BlockBlobStorage" and new feature fileaadIntegration #3864

Merged
merged 13 commits into from
Sep 14, 2018

Conversation

blueww
Copy link
Member

@blueww blueww commented Sep 11, 2018

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

Rest Spec: https://microsoft.sharepoint.com/teams/AzureStorage/_layouts/15/Doc.aspx?sourcedoc=%7B00d45481-7f25-4170-a95e-82dbc6a3041d%7D&action=default&slrid=0d385f9e-a0fa-0000-758b-e5dde88dbd09

Pass review on private repo:
New Kind: https://github.com/Azure/azure-rest-api-specs-pr/pull/538
FileAAdIntegration: https://github.com/Azure/azure-rest-api-specs-pr/pull/508

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#3352

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

"tags": [
"Usage"
],
"operationId": "Usage_List",
Copy link
Member

Choose a reason for hiding this comment

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

Why this renaming from "Usages" to "Usage"? That's a SDK breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, just see that it was "Usage", and then it was renamed in 2018-03-01-preview as "Usages", and now rename back to "Usage". So I guess it's ok

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be usages. I will fix it.

@lmazuel
Copy link
Member

lmazuel commented Sep 11, 2018

Hi @blueww
What is the status of 2018-03-01-preview? I see that management policies methods are not here in this PR. Does it mean it's missing here (and PR needs to be updated), or that management policies should be still called from 2018-03-01-preview? If the later is the truth, this means there is some Readme configuration to do to mix-up 2018-03-01-preview/2018-07-01 for SDK release.

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#3605

},
"isHnsEnabled": {
"type": "boolean",
"default": false,
Copy link
Member

Choose a reason for hiding this comment

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

Based on a recent issue found by @williexu in the CLI, please also remove this default value.

},
"supportsHttpsTrafficOnly": {
"type": "boolean",
"default": false,
Copy link
Member

Choose a reason for hiding this comment

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

Based on a recent issue found by @williexu in the CLI, please also remove this default value.

@blueww
Copy link
Member Author

blueww commented Sep 12, 2018

@lmazuel
Management policies is not supported on 2018-07-01, it's only on 2018-03-01-preview now, so there will be a breaking change.
I have discussed with Management policies feature PM and SRP team, and get agreement not support Management policies on 2018-07-01 and the SRP SDK based on it.

We will add back Management policies to a stable api version when the feature GA in the future.

@blueww
Copy link
Member Author

blueww commented Sep 13, 2018

@lmazuel
I have added the Management policy feature back. And remove Global Usage list since not supported in new api version (as location usage added).
Please help to review.

input-file:
- Microsoft.Storage/stable/2018-07-01/storage.json
- Microsoft.Storage/stable/2018-07-01/blob.json
- Microsoft.Storage/stable/2018-07-01/managementpolicy.json
Copy link
Member

Choose a reason for hiding this comment

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

Please put the new split file in “2018-03-01-preview” folder, and just reference it from here:

  • Microsoft.Storage/stable/2018-07-01/storage.json
  • Microsoft.Storage/stable/2018-07-01/blob.json
  • Microsoft.Storage/stable/2018-03-01-preview/managementpolicy.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@blueww
Copy link
Member Author

blueww commented Sep 14, 2018

@lmazuel
I have fixed the management policy swagger location. And for Usage -> Usages, it's required by OneSDK team since "Using non-plural name for operation group is against SDK guideline ", I think we should keep using "Usages".
I also added new Sku "Premium_ZRS" per feature team requirement.

Since the release is target Ignite, and we need to raise and review SDK PR after the swagger PR is in, would you please help to review and merge the PR asap if no other issues.

Thanks very much!

@lmazuel
Copy link
Member

lmazuel commented Sep 14, 2018

@hovsepm , since you already approved, merging the PR. Thanks!

@lmazuel lmazuel merged commit 8d4e4c2 into Azure:master Sep 14, 2018
@hassanbabaie
Copy link

@blueww Hi Wei, just a quick question I see this AAD support coming to AzureFiles, is this the same as what was just released at ignite as that also required the use of Azure Active Directory Domain Services which is a real pain compared to say the new AAD for object storage which doesn't

Please let me know your thoughts

Hass

@blueww
Copy link
Member Author

blueww commented Oct 8, 2018

@hassanbabaie
You can find more details for the feature in https://azure.microsoft.com/en-us/blog/azure-active-directory-integration-for-smb-access-now-in-public-preview/
If you have any question or suggestions for this feature, you can contact AzureFiles@microsoft.com

@hassanbabaie
Copy link

Thank you for the quick response and that answers my questions as it still uses Azure AD Domain Services (AAD DS) however I will also follow up to the e-mail address with feedback

Hass

mccleanp pushed a commit that referenced this pull request Mar 23, 2022
…#3864)

* Added sku/dev (for MIAA/PG) and uploadWatermark (for data controller) in swagger

* set proper default boolean value for sku.dev

* fix an issue flagged by prettier

Co-authored-by: Yuqing Li <yuqingli@ntdev.microsoft.com>
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.

6 participants