Azure/azure-rest-api-specs

[RiskIQ - EASM - Defender EASM] API Review

azure-sdk opened this issue · 4 comments

New API Review meeting has been requested.

Service Name: RiskIQ - EASM - Defender EASM
Review Created By: Dasha Spencer
Review Date: 10/23/2024 09:00 AM PT
Release Plan:
PR: #31045
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description:

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

Meeting updated by Dasha Spencer

Service Name: RiskIQ - EASM - Defender EASM
Review Created By: Dasha Spencer
Review Date: 10/23/2024 09:00 AM PT
Release Plan:
PR: #31045
Hero Scenarios Link: here
Core Concepts Doc Link: Not Provided

Description:

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

Notes from API Review 10/23/2024

  • Revisit CreateOrUpdate vs CreateOrReplace
  • clientNames should be defined in client.tsp
  • All the fields inside PolicyData are optional -- is that intentional?
    • No -- action and filter names should be required
  • Priority is not being used -- leave it out
  • ActionParameters -- seems to be too loosely defined
    • Are names case sensitive?
    • Can name contain white-space? special characters?
    • Can values only be strings?
    • Are name and value really optional?
    • Add a link to the docs
  • Best to have a single model that is uses for both request and response in PUT, PATCH, etc.
  • Document that date-time values should follow RFC 3339 format
  • Is the kind in ObservationRemediationItem a discriminator, or just an extensible enum?
    • Just an enum
  • Please fix all properties with missing descriptions
  • Need to plan for a GA version soon

This is okay for preview, but please consider the above and make whatever changes you can here, and otherwise in the next (hopefully GA) version.

@mikekistler Thank you for the feedback! Working on addressing the above items in the next few days.

First wave (today):

  • Move clientNames annotation into client.tsp
  • Properly mark fields in PolicyData as optional vs not
  • Remove unused param, including Priority
  • Add docs link for the ActionParameters which should provide more context on what values to actually use
  • Document that date-time values should follow RFC 3339 format

Second wave TODO (probably tomorrow):

  • Best to have a single model that is uses for both request and response in PUT, PATCH, etc.

Items left until closer to the GA release - These changes will not be addressed in this PR:

  • Revisit CreateOrUpdate vs CreateOrReplace
  • Please fix all properties with missing descriptions
  • Need to plan for a GA version soon

@mikekistler I have made all the tsp changes I expect to make. Could you please take another look and let me know if you see any issues? I do not have permissions to add the labels the build requires based on the feedback in the "Next steps to merge" section of the PR. I also am not sure of the proper procedures in this case to for introducing "breaking" changes. I believe this can be done, but again I do not have the permissions

Note
If your changes to the REST API definition are correcting the definition to accurately describe the service behavior, you can bypass the review process by adding appropriate BreakingChange-Approved-BugFix or Versioning-Approved-BugFix label to your pull request.