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

Add CustomEnityLookupSkill to 2020-06-30 and 2020-06-30-Preview #11113

Merged
merged 16 commits into from
Jan 6, 2021
Merged

Add CustomEnityLookupSkill to 2020-06-30 and 2020-06-30-Preview #11113

merged 16 commits into from
Jan 6, 2021

Conversation

shuyangmsft
Copy link
Contributor

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

If any further question about AME onboarding or validation tools, please view the FAQ.

ARM API Review Checklist

  • Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.

    • Adding new API(s)
    • Adding a new API version
    • Adding a new service

  • If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist

If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.

  • Removing API(s) in stable version
  • Removing properties in stable version
  • Removing API version(s) in stable version
  • Updating API in stable version with Breaking Change Validation errors
  • Updating API(s) in preview over 1 year

Please follow the link to find more details on PR review process.

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Oct 9, 2020 •
edited
Loading

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand] 
There are no breaking changes.

️️✔️LintDiff succeeded [Detail] [Expand] 
Validation passes for LintDiff.

️️✔️Avocado succeeded [Detail] [Expand] 
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand] 
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand] 
Validation passes for SemanticValidation.
️️✔️[Staging] Cross Version BreakingChange (Base on preview version) succeeded [Detail] [Expand] 
There are no breaking changes.
️️✔️[Staging] Cross Version BreakingChange (Base on stable version) succeeded [Detail] [Expand] 
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand] 
There is no credential detected.
Posted by Swagger Pipeline | How to fix these errors?

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

Azure CLI Extension Generation

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

azure-sdk-for-js

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

azure-sdk-for-java

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

azure-resource-manager-schemas

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

azure-sdk-for-python

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

Trenton Generation

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

azure-sdk-for-net

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 9, 2020 •
edited
Loading

azure-sdk-for-go

No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

@shuyangmsft shuyangmsft marked this pull request as draft October 9, 2020 21:18
heaths
heaths requested changes Oct 9, 2020
View reviewed changes
specification/search/data-plane/Azure.Search/preview/2020-06-30/searchservice.json
@@ -6943,6 +6943,92 @@
},
"description": "Base type for skills."
},
"CustomEntity": {
Copy link
Member

Choose a reason for hiding this comment

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

2020-06-30 already GA'd. Unless this support already existed and just wasn't formalized in swagger, it should only be in 2020-06-30-Preview, which will change dates when closer to its GA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually have a question here given our case is a little bit involved.
The CustomEntityLookupSkill is publicly available. It's currently in our preview API and customers are using it right now.
The skill was added a while ago so it's not in the preview swagger.
Now our team wants to GA the feature so it should live in both the GA API and the preview API.
Given the situation we are at, should I remove or keep the 2020-06-30 change?

Copy link
Member

Choose a reason for hiding this comment

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

/cc @johanste @brjohnstmsft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi team, can I get any update on this?

Copy link
Member

Choose a reason for hiding this comment

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

FYI @bleroy since this is about Skillsets

Copy link
Contributor

Choose a reason for hiding this comment

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

@heaths of course. We are still getting used to navigating this new world where the SDK team is so involved and all of the new breaking change requirements. We are definitely considering the SDK implications with the new skill versioning discussions. But since those discussions are ongoing and the work won't be done until Cobalt, I think taking a minor version bump for a change such as this one is acceptable for now.

Copy link
Member

Choose a reason for hiding this comment

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

We're going to GA a minor version bump in a few weeks. @AlexGhiondea would adding a model require going through at least a single preview, or would this be minor enough (no pun intended) to include in our upcoming GA?

Copy link
Member

Choose a reason for hiding this comment

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

@johanste how should this be handled going forward if new models are added to GAs? Isn't there roundtripping concerns here as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there are some round-tripping concerns here. A client that knows nothing about a new skill type can easily end up stripping out properties it did not know about on update (assuming PUT does a replace rather than a merge-patch like update). I believe that I and @JeffreyRichter can be of assistance when it comes to determining (or at least reviewing) what a good versioning model for your service APIs should be.
Likewise, as mentioned above,

Copy link
Contributor

Choose a reason for hiding this comment

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

  • @jennifermarsman since I know she was talking about revisting this design in the coming weeks. We are hoping to finalize the design in Iron with the intent of implementing it in Cobalt, and we would definitely value any feedback from the SDK folks as we do so, so perhaps we should loop ya'll into our thread we have on the topic.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tjprescott
Copy link
Member

@shuyangmsft is this ready to merge?

@shuyangmsft
Copy link
Contributor Author

@shuyangmsft is this ready to merge?

@shuyangmsft shuyangmsft reopened this Oct 26, 2020
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@shuyangmsft
Copy link
Contributor Author

@shuyangmsft is this ready to merge?

Hi Travis, our backend change is in pipeline. I'll let you know when it's deployed to prod. Thanks!

@heaths
Copy link
Member

heaths commented Oct 26, 2020

Any ETA? We have an SDK release coming up very soon and still need to regenerate source based on this change.

@shuyangmsft
Copy link
Contributor Author

Any ETA? We have an SDK release coming up very soon and still need to regenerate source based on this change.

Based on our pipeline status, we definitely can't make this change to prod this week. Next week should work. I'll keep you updated.

@heaths heaths added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Oct 30, 2020
@heaths
Copy link
Member

heaths commented Oct 30, 2020

Please hold off on merging this. We won't be able to generate and test this sufficiently for the upcoming release and have decided not to include it. It will have to be included in the next beta. This can be merged after we code freeze next week.

@ghost
Copy link

ghost commented Dec 27, 2020

Hi, @shuyangmsft. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove no-recent-activity label.

@ghost ghost added the no-recent-activity label Dec 27, 2020
@heaths heaths removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label Jan 5, 2021
@tjprescott
Copy link
Member

/azp run

@openapi-pipeline-app
Copy link

openapi-pipeline-app bot commented Jan 5, 2021 •
edited
Loading

Swagger pipeline restarted successfully, please wait for status update in this comment.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tjprescott tjprescott merged commit ae379e1 into Azure:master Jan 6, 2021
@heaths heaths mentioned this pull request Feb 10, 2021
Closed
@Mohit-Chakraborty Mohit-Chakraborty mentioned this pull request Mar 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Careyjmac Careyjmac Careyjmac left review comments

@johanste johanste johanste left review comments

@bleroy bleroy bleroy approved these changes

@heaths heaths heaths approved these changes

@tjprescott tjprescott tjprescott approved these changes

@AlexGhiondea AlexGhiondea Awaiting requested review from AlexGhiondea

@arv100kri arv100kri Awaiting requested review from arv100kri arv100kri is a code owner

@brjohnstmsft brjohnstmsft Awaiting requested review from brjohnstmsft

Assignees

@tjprescott tjprescott

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shuyangmsft @tjprescott @heaths @bleroy @brjohnstmsft @Careyjmac @johanste