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 #31056

Closed
azure-sdk opened this issue Oct 16, 2024 · 4 comments · Fixed by #31045
Closed

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

azure-sdk opened this issue Oct 16, 2024 · 4 comments · Fixed by #31045
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: 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

@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 Oct 16, 2024
@azure-sdk
Copy link
Collaborator Author

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

@mikekistler mikekistler linked a pull request Oct 23, 2024 that will close this issue
3 tasks
@mikekistler
Copy link
Member

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.

@dashaspencer
Copy link
Member

dashaspencer commented Oct 23, 2024

@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

@dashaspencer
Copy link
Member

@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.

@github-project-automation github-project-automation bot moved this from Triage to Done in API Stewardship Oct 28, 2024
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