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.

Sign up for GitHub

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

Jump to bottom

fix schema property naming #6901

Merged
merged 2 commits into from
Aug 14, 2019
Merged

fix schema property naming #6901

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be17c03
fix schema property naming
alargeasteroid Aug 8, 2019
faa8f71
remove redundent schema reference
alargeasteroid Aug 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions specification/cognitiveservices/data-plane/QnAMaker/stable/v4.0/QnAMaker.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,18 +828,6 @@
}
}
},
"KnowledgebaseEnvironment": {
Copy link
Member

Choose a reason for hiding this comment

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

Is this removal considered as a breaking change? Breaking changes should not happen in a stable version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that this is only used once for this file's redundant schema reference. I would assume that removing this has no breaking impact on the schema. Are you aware of a reason this would be a 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.

So you are saying that removal of this schema will not cause a breaking change. If so, I am ok with this, and will approve this as soon as azure api board responds

Copy link
Member

Choose a reason for hiding this comment

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

Is there a change to the REST API that motivated this change?

Copy link
Member

Choose a reason for hiding this comment

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

Per offline conversation, this is not a change to the public REST API. Thus, no need for the Azure API review board to review.

Copy link
Member

@ArcturusZhang ArcturusZhang Aug 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @alargeasteroid after a check on SDK, this removal will cause a breaking change on go SDK (we have a tool for that). But in my opinion, this situation happens day to day, we always get unexpected breaking changes in SDK and in semver we will hold this kind of breaking change to next major version release

"type": "string",
"description": "Enumeration of knowledgebase environments.",
"x-ms-enum": {
"name": "KnowledgebaseEnvironmentType",
"modelAsString": true
},
"enum": [
"Prod",
"Test"
]
},
"QnADocumentsDTO": {
"type": "object",
"description": "List of QnADTO",
Expand Down Expand Up @@ -1346,9 +1334,6 @@
"name": "environment",
"in": "path",
"required": true,
"x-schema": {
"$ref": "#/definitions/KnowledgebaseEnvironment"
},
"x-nullable": false,
"description": "Specifies whether environment is Test or Prod.",
"x-ms-enum": {
Expand Down