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

[Cognitive Services] Update endpoint URL template for Content Moderator. #3505

Merged
merged 2 commits into from Aug 10, 2018
Merged

[Cognitive Services] Update endpoint URL template for Content Moderator. #3505

merged 2 commits into from Aug 10, 2018

Conversation

yangyuan
Copy link
Member

@yangyuan yangyuan commented Jul 25, 2018

Detail background and explainations in here: #3489 Cognitive Services URL template (endpoint)

Please hold for swiftarrow11 or sanjeev3's review as well as my confirmation.

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

@yangyuan yangyuan requested a review from swiftarrow11 July 25, 2018 21:49
@@ -8,7 +8,6 @@ nodejs:
package-name: azure-cognitiveservices-contentmoderator
package-version: 1.0.0-preview
output-folder: $(node-sdks-folder)/lib/services/contentModerator
override-client-name: ContentModeratorAPIClient
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this to keep consistency

@@ -3,7 +3,7 @@
"info": {
"title": "Content Moderator Client",
"version": "1.0",
"description": "You use the API to scan your content as it is generated. Content Moderator then processes your content and sends the results along with relevant information either back to your systems or to the built-in review tool. You can use this information to take decisions e.g. take it down, send to human judge, etc.\r\n\r\nWhen using the API, images need to have a minimum of 128 pixels and a maximum file size of 4MB. \r\nText can be at most 1024 characters long. \r\nIf the content passed to the text API or the image API exceeds the size limits, the API will return an error code that informs about the issue.\r\n\r\nThis API is currently available in:\r\n\r\n* West US - westus.api.cognitive.microsoft.com\r\n* East US 2 - eastus2.api.cognitive.microsoft.com\r\n* West Central US - westcentralus.api.cognitive.microsoft.com\r\n* West Europe - westeurope.api.cognitive.microsoft.com\r\n* Southeast Asia - southeastasia.api.cognitive.microsoft.com ."
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove the outdated region list.

@AutorestCI
Copy link

AutorestCI commented Jul 25, 2018

Automation for azure-sdk-for-java

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

@AutorestCI
Copy link

AutorestCI commented Jul 25, 2018

Automation for azure-sdk-for-go

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

@AutorestCI
Copy link

AutorestCI commented Jul 25, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Jul 25, 2018

Automation for azure-sdk-for-ruby

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

@AutorestCI
Copy link

AutorestCI commented Jul 25, 2018

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3238

@yangyuan yangyuan requested a review from sanjeev3 July 26, 2018 00:46
@yangyuan yangyuan added In-Review DoNotMerge <valid label in PR review process> use to hold merge after approval and removed In-Review labels Jul 26, 2018
@jhendrixMSFT
Copy link
Member

@yangyuan are we still waiting for other reviewers?

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@yangyuan
Copy link
Member Author

yangyuan commented Aug 2, 2018

Hi @jhendrixMSFT. I've updated some SDKs, but heard some negative feedback (also found a small issue in auto-generated documents).
So I decide to hold this PR for a few days and collect feedbacks in case of back/forth changes.

@sanjeev3
Copy link

sanjeev3 commented Aug 7, 2018

@yangyuan @jhendrixMSFT, I am good with these. @swiftarrow11, how about you?

Copy link
Member

@jhendrixMSFT jhendrixMSFT left a comment

Choose a reason for hiding this comment

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

There are some model validation failures that need to be addressed, please see the travis log.

@yangyuan yangyuan removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label Aug 7, 2018
@yangyuan
Copy link
Member Author

yangyuan commented Aug 8, 2018

hi @jhendrixMSFT, I've fixed the model validation error.

It shows that Linter Diff complaints some operations missing x-ms-examples.
How about we create an issue and deal with it later? two reasons I recommend that.

  1. Its some legacy issues not related to this change.
  2. The examples should be provided by Content Moderator team.

@jhendrixMSFT
Copy link
Member

@bsiegel I understand you're working on this new linter diff in CI? I don't think it should be failing for this PR as it doesn't add any new operations.

@jhendrixMSFT
Copy link
Member

@veronicagg do you know about this new linter behavior?

@veronicagg
Copy link
Contributor

@jhendrixMSFT I asked @bsiegel about this in the morning. @bsiegel I think this is another case for the issue you fixed earlier today, in which case this branch would need to be rebased to pick up Brandon's fix. @bsiegel could you check other PRs that may be opened and got affected by it?

@yangyuan
Copy link
Member Author

@jhendrixMSFT I have rebase the branch and now all passes. :)

Copy link
Member

@jhendrixMSFT jhendrixMSFT left a comment

Choose a reason for hiding this comment

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

There are some model validation failures that need to be addressed, please see the travis log.

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