Content Moderator new version changes by v-sodsou · Pull Request #2317 · Azure/azure-rest-api-specs

v-sodsou · 2018-01-23T19:31:35Z

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.

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

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

lmazuel

Stable/Preview folder

lmazuel · 2018-01-24T21:30:36Z

specification/cognitiveservices/data-plane/ContentModerator/readme.md

+``` yaml $(tag) == 'release_1_1'
 input-file: 
-  - stable/v1.0/ContentModerator.json
+  - v1.1/ContentModerator.json


Stable/Preview has to be used.

fearthecowboy

@v-sodsou

Before I get anywhere with the review, a couple salient points:

Don't remove old revisions, create new ones and add new sections to the configurations. Never remove an existing revision. The configuration can be set to only generate a given version, but we need to preserve everything. If the examples haven't changed, don't move the files, reference them from the old location. If an example is new or changed, create it in the appropriate new folder location.
Follow the file-folder layout that is in the repository. We recently introduced a stable folder in the path, keep the files in the right places.

fearthecowboy · 2018-01-24T21:25:38Z

specification/cognitiveservices/data-plane/ContentModerator/readme.md


-### Release 1.0
-These settings apply only when `--tag=release_1_0` is specified on the command line.
+### Release 1.1


Do not remove old releases!

Create a new release section and change the default, but don't remove the old one.

fearthecowboy

Please make this change additive, not replacing or moving the existing revision, and then I can continue

lmazuel · 2018-01-24T22:49:51Z

Just to be clear, I approved "stable/preview" folder, nothing else. Just to be sure I won't lock the PR when @fearthecowboy is done.

fearthecowboy · 2018-01-25T21:23:01Z

specification/cognitiveservices/data-plane/ContentModerator/stable/v1.1/ContentModerator.json

+    "/contentmoderator/lists/v1.0/imagelists/{listId}/images?overload=stream": {
+      "post": {
+        "tags": [
+          "ListManagemen


It appears that this is the only significant change in the API.

As far as APIs go, having this hardcoded to Category1/2/3 seems ... inefficient and a bit clumsy. Is it possible to use an array of Scores instead?

@fearthecowboy For API , naming properties Category1/2/3 was due to our CELA lawyers. For more information on this you can reach out to Greg Clark (gregc@microsoft.com) .

Regarding returning an array instead rather than class, we chose to return class instead array so that we can add more fields in future and traverse the fields easily.

@fearthecowboy Do you see any concerns?

It's just a rather rigid way to express something that looks like it could potentially have more than the arbitrary 3 that you've got there. What happens if in the future, you want four?

With an array instead, you can send an arbitrary number back, and the SDK won't change when you do that.

You could just have a Categories property that is an array of Score instead of separate individual properties.

Our goal here is to make sure that generated SDKs are convenient and flexible for our customers, so we can minimize the impact of API changes later on.

@swiftarrow11 can provide you more information on why it is implemented as class than an array.

fearthecowboy

I'm curious if the API can be refactored to just return an array rather than a class that has fixed fields for category1/2/3

fearthecowboy · 2018-01-26T19:14:30Z

@v-sodsou -- the other thing that @lmazuel brought to my attention, is that you've called this 1.1 but the paths all still point to 1.0

The path /contentmoderator/moderate/v1.0/ProcessText/Screen/ in the existing spec returns a different structure. If you change the implementation of the existing endpoint, you will break existing SDK/clients.

You should be either changing the path for that API in the 1.1 version to /contentmoderator/moderate/v1.1/ProcessText/Screen/ (or more likely do the whole set).

What exactly are you plans for rolling out this API version into production? Are you supporting multiple API versions concurrently?

v-sodsou · 2018-01-26T19:45:50Z

@fearthecowboy we have rolled out this "/contentmoderator/moderate/v1.0/ProcessText/Screen/" API version as v1.0 into production. So I have retained the same in specification file. Earlier this classify feature was in preview mode and only few customers were using it. Now that it is public , customers who are using it , are already notified about this change. Perhaps, I shouldn't have added this feature in earlier version since it was in preview. Apologies for the confusion caused.

lmazuel · 2018-01-26T19:58:14Z

@v-sodsou If I understand correctly what you are saying, you should not create a virtual 1.1 that doesn't exists, but update the current v1.0 with the truth. There is no point for anyone to keep trace of a v1.0 folder that does not represent anything in the real world. At worst, we can get the Swagger as it was using Git (that the point of a versionning system).

v-sodsou · 2018-01-26T20:12:46Z

@lmazuel @fearthecowboy
We have just updated the response schema of this API and rolled out into prod as "/contentmoderator/moderate/v1.0/ProcessText/Screen/" .
Ideally could you tell me how to proceed with specification and sdk changes in this situation?

1.Should I not change the version of specification file?
2.Should I not create new folder v1.1 ?
I was asked to create a new version in my previous pull request for this.Hence I created new version of it in specification repository.

Could you tell me the right way to proceed.

fearthecowboy · 2018-01-26T20:16:06Z

We wanted you to create a new version, because you changed the version number in the configuration, which made us think that you were introducing a new API version.

Given that this is not a new version at all, you're just overwriting an existing API definition in an existing API version, we now suggest that you keep everything at 1.0

v-sodsou · 2018-01-26T20:21:06Z

@fearthecowboy Sure. I'll keep everything at 1.0 then. Thanks.

fearthecowboy · 2018-01-29T22:07:24Z

I think we should be clear for the future; you should at all cost avoid changing an API definition for an existing API -- once a client is shipped, it's really dangerous for us to alter the API.

If that's absolutely necessary, then I would recommend that you do change the API version and support multiple concurrent API versions so that customers are not broken.

If an API is not considered "stable" (and you will be updating it in production), then we should move the spec to the preview folder until it is considered stable. That way we can tag appropriate SDKs as 'preview' so customers have a better expectation

azuresdkciprbot · 2018-01-29T22:28:15Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/cognitiveservices/data-plane/ContentModerator/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

v-sodsou · 2018-01-29T22:38:28Z

@fearthecowboy Thank you for the information and assistance. I have conveyed this to my team as well. We shall keep this in mind when we have future changes in API.

v-sodsou · 2018-01-29T22:45:44Z

@fearthecowboy Build job for Mode=Semantic PR_Only is errored. Can you help me resolve this error.

AutorestCI · 2018-01-29T23:18:27Z

This commit was treated and no generation was made for Azure/azure-sdk-for-go

AutorestCI · 2018-01-29T23:30:45Z

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@8909170

fearthecowboy · 2018-01-30T15:16:12Z

@v-sodsou I've restarted the CI task. Looks like it went crazy. Looks ok to me now.

If we're OK with this I can merge it.

AutorestCI · 2018-01-31T00:36:40Z

This commit was treated and no generation was made for Azure/azure-sdk-for-go

AutorestCI · 2018-01-31T00:37:10Z

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@639b61d

azuresdkciprbot · 2018-01-31T00:39:31Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/cognitiveservices/data-plane/ContentModerator/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

AutorestCI · 2018-02-01T23:05:33Z

This commit was treated and no generation was made for Azure/azure-sdk-for-go

AutorestCI · 2018-02-01T23:05:50Z

Swagger to SDK encountered an unknown error: (Azure/azure-sdk-for-go)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/github_tools.py", line 29, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 180, in rest_handle_action
    return rest_pull_close(body, github_con, restapi_repo, sdk_pr_target_repo, sdkbase)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 311, in rest_pull_close
    rest_pr.create_issue_comment("Was unable to create SDK %s PR for this closed PR.", sdkid)
TypeError: create_issue_comment() takes 2 positional arguments but 3 were given

v-sodsou added 2 commits January 23, 2018 11:23

create new version content moderator v1.1

d1e7929

Changed the spec version

96d1130

azuresdkci assigned fearthecowboy Jan 24, 2018

lmazuel requested changes Jan 24, 2018

View reviewed changes

moved v1.1 to stable folder

360e191

fearthecowboy reviewed Jan 24, 2018

View reviewed changes

fearthecowboy suggested changes Jan 24, 2018

View reviewed changes

lmazuel approved these changes Jan 24, 2018

View reviewed changes

Changes as per PR comments

1b61171

fearthecowboy reviewed Jan 25, 2018

View reviewed changes

fearthecowboy suggested changes Jan 25, 2018

View reviewed changes

Revert version changes

b594737

fearthecowboy approved these changes Jan 30, 2018

View reviewed changes

Missing Python version

d18cc9d

lmazuel mentioned this pull request Jan 31, 2018

Restapi auto v sodsou content moderator v2 Azure/azure-sdk-for-python#1861

Merged

fearthecowboy merged commit 8847e80 into Azure:master Feb 1, 2018

Conversation

v-sodsou commented Jan 23, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

lmazuel left a comment

Choose a reason for hiding this comment

Uh oh!

lmazuel Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy left a comment

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy left a comment

Choose a reason for hiding this comment

Uh oh!

lmazuel commented Jan 24, 2018

Uh oh!

fearthecowboy Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

v-sodsou Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-sodsou Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

v-sodsou Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-sodsou Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

fearthecowboy left a comment

Choose a reason for hiding this comment

Uh oh!

fearthecowboy commented Jan 26, 2018

Uh oh!

v-sodsou commented Jan 26, 2018

Uh oh!

lmazuel commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

v-sodsou commented Jan 26, 2018

Uh oh!

fearthecowboy commented Jan 26, 2018

Uh oh!

v-sodsou commented Jan 26, 2018

Uh oh!

fearthecowboy commented Jan 29, 2018

Uh oh!

azuresdkciprbot commented Jan 29, 2018

Uh oh!

v-sodsou commented Jan 29, 2018

Uh oh!

v-sodsou commented Jan 29, 2018

Uh oh!

AutorestCI commented Jan 29, 2018

Uh oh!

AutorestCI commented Jan 29, 2018

Uh oh!

fearthecowboy commented Jan 30, 2018

Uh oh!

AutorestCI commented Jan 31, 2018

Uh oh!

AutorestCI commented Jan 31, 2018

Uh oh!

azuresdkciprbot commented Jan 31, 2018

Uh oh!

AutorestCI commented Feb 1, 2018

v-sodsou commented Jan 23, 2018 •

edited by azuresdkci

Loading

v-sodsou Jan 25, 2018 •

edited

Loading

fearthecowboy Jan 26, 2018 •

edited

Loading

lmazuel commented Jan 26, 2018 •

edited

Loading