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

[RiskIQ - EASM - Defender EASM] API Review #24887

Closed
azure-sdk opened this issue Jul 18, 2023 · 11 comments · Fixed by #24937
Closed

[RiskIQ - EASM - Defender EASM] API Review #24887

azure-sdk opened this issue Jul 18, 2023 · 11 comments · Fixed by #24937
Assignees
Labels
API Review Scoping This is an issue that will track work on a specific set of API changes.

Comments

@azure-sdk
Copy link
Collaborator

New API Review meeting has been requested.

Service Name: RiskIQ - EASM - Defender EASM
Review Created By: Thang Le
Review Date: 07/26/2023 09:00 AM PT
Onboarding Record: https://dev.azure.com/azure-sdk/Release/_workitems/edit/12054
PR:
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description: Add a Typespec file for api version 2023-03-01-preview

Detailed meeting information and documents provided can be accessed here
For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

@azure-sdk azure-sdk added the API Review Scoping This is an issue that will track work on a specific set of API changes. label Jul 18, 2023
@azure-sdk
Copy link
Collaborator Author

Meeting updated by Thang Le

Service Name: RiskIQ - EASM - Defender EASM
Review Created By: Thang Le
Review Date: 07/26/2023 09:00 AM PT
Onboarding Record: https://dev.azure.com/azure-sdk/Release/_workitems/edit/12054
PR:
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description: Add a Typespec file for api version 2023-03-01-preview

Detailed meeting information and documents provided can be accessed here
For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

1 similar comment
@azure-sdk
Copy link
Collaborator Author

Meeting updated by Thang Le

Service Name: RiskIQ - EASM - Defender EASM
Review Created By: Thang Le
Review Date: 07/26/2023 09:00 AM PT
Onboarding Record: https://dev.azure.com/azure-sdk/Release/_workitems/edit/12054
PR:
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description: Add a Typespec file for api version 2023-03-01-preview

Detailed meeting information and documents provided can be accessed here
For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

@mikekistler
Copy link
Member

@thang-bit Please add a link to the PR to be reviewed here, or if there is no PR any other materials we can use to prep for the meeting.

@azure-sdk
Copy link
Collaborator Author

Meeting updated by Thang Le

Service Name: RiskIQ - EASM - Defender EASM
Review Created By: Thang Le
Review Date: 07/26/2023 09:00 AM PT
Onboarding Record: https://dev.azure.com/azure-sdk/Release/_workitems/edit/12054
PR: #24937
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description: Add a Typespec file for api version 2023-03-01-preview

Detailed meeting information and documents provided can be accessed here
For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

@thang-bit
Copy link
Member

@mikekistler I have added the PR, thank you for the reminder

@mikekistler
Copy link
Member

Notes from API Review meeting 7/26/23

  • Why is this functionality in data plane rather than control plane ?
    • Historical decision, but it would be a lot of work to rearchitect the product
  • It would be best to use Azure Core, or at the very least define your specific operation patterns rather that have each operation defined on its own.
    • Need to schedule time with the typespec team to figure out what can be done here
  • Should use 'url' typespec type for fields that are URLs
  • Consider using generics in TypeSpec to make your API definition more concise

@thang-bit
Copy link
Member

@mikekistler where should I put the swagger file generated from typespec in this pull request?

@mikekistler
Copy link
Member

It goes in the standard place. If you are using the same API version as your existing spec then it will just overwrite that file.

@thang-bit
Copy link
Member

thang-bit commented Aug 14, 2023

@mikekistler I have updated the PR based on the comments of API review board and suggestions from the typespec team.
The changes include:

  • Use Typespec Azure.Core standard operations
  • Use typespec url type for URLs
  • Replace existing swagger file with the swagger file generated from typespec
  • Replace the current examples with examples to represent the updated swagger file

Since the updated swagger file resulted in multiple breaking changes. Would it be required to have another review meeting go over breaking changes? Or is it sufficient to review things offline?

@thang-bit
Copy link
Member

@mikekistler I have updated the PR based on your comments on the typespec file.
The changes include:

  • Add comments to remind us to add documentation on properties in the next review
  • Update some models name to camelCase style
  • Update "Put" in method name to "CreateOrReplace"
  • Update response model that has "metadata" properties to readonly
    We didn't update the createOrReplace to have 201 response when a resource is created since this version was published and available for public. However, we will ensure this will be addressed in later review

@thang-bit
Copy link
Member

Hi @mikekistler, the PR is approved, but it isn't merged to the main branch, is it because of the breaking change tag and CI-fixedRequiredOnFailure tag? If so, how can we address these?

@github-project-automation github-project-automation bot moved this from Triage to Done in API Stewardship Oct 19, 2023
@thang-bit thang-bit linked a pull request Feb 22, 2024 that will close this issue
3 tasks
@thang-bit thang-bit removed a link to a pull request Feb 22, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Review Scoping This is an issue that will track work on a specific set of API changes.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants