Skip to content

Conversation

waldekmastykarz
Copy link
Collaborator

Adds detecting minimal permissions. Closes #57

@waldekmastykarz waldekmastykarz requested a review from a team as a code owner May 12, 2023 12:35
Copy link
Contributor

@sebastienlevert sebastienlevert left a comment

Choose a reason for hiding this comment

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

My only request would be to utilize a kiota generated client to access the DevX API. Let's dogfood as much as possible!

@waldekmastykarz
Copy link
Collaborator Author

My only request would be to utilize a kiota generated client to access the DevX API. Let's dogfood as much as possible!

Do we have it already? If not, do we have all the metadata necessary to generate it, since the API is just in preview?

@gavinbarron
Copy link
Contributor

Hey @sebastienlevert while I like the idea of dogfooding kiota here, I'm not sure that this is the right project for it.
It's very tempting from an engineering perspective to want to add this dependency, but that comes with costs:

  • Engineering time to generate the client (if we can)
  • Engineering time to integrate the additional dependencies including an anonymous auth provider for the request adapter
  • Documentation time to make it easy for other contributors to work on this.
  • Testing & Engineering cycles to validate the build/release process now that the plugins have additional dependencies that are not provided by the mgdp executable itself.
  • Dependency churn increasing our dev effort every month as Kiota releases new versions consuming more engineering effort.

And all for one web request, one that we don't know if it has an Open API definition to generate from. It's a no from me at present.
I'd have a different opinion if this were for multiple APIs on the same domain or we had more time to devote to this project.

@waldekmastykarz waldekmastykarz enabled auto-merge (squash) May 22, 2023 07:48
@waldekmastykarz waldekmastykarz disabled auto-merge May 22, 2023 16:19
@sebastienlevert
Copy link
Contributor

We should have an OpenAPI for it. I understand your concerns, I still would love us to use Kiota here. These concerns are the concerns of any devs wanting to leverage an API. We are no different. If we vote that this is too much, then I think we failed with Kiota. Again, it's not a blocking issue, but we should be looking it at from the angle of let's validate our own patterns.

@darrelmiller
Copy link

OpenAPI for devX API is here https://graphexplorerapi.azurewebsites.net/swagger/index.html @jasonjoh generated a Kiota client DevX API a long time ago.

@gavinbarron
Copy link
Contributor

At the moment the current version of the OpenAPI definition is not correct for the permissions endpoint based on the shape of the returned object from a POST to permissions
Also a POST to the permissions endpoint on prod results in a 404 at present.

Given that this is not in Prod yet we may need to hold this change until we have the change to the DevX API in Prod.

@gavinbarron
Copy link
Contributor

To move this forward, we're going to release a preview version of this plugin in the next release,.
We will address the migration to Kiota in #263 alongside the change to using the production version of the API in #264 when the DevX team release this change to production.

@waldekmastykarz waldekmastykarz modified the milestone: v0.8.0 May 25, 2023
@waldekmastykarz waldekmastykarz dismissed sebastienlevert’s stale review May 30, 2023 09:21

We agreed to move forward with this PR and then address migration to Kiota in the future release

@waldekmastykarz waldekmastykarz merged commit 5b20124 into dotnet:main May 30, 2023
@waldekmastykarz waldekmastykarz deleted the minimal-permissions branch May 30, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help developers determine the minimal set of permissions for the (portion of the) app
5 participants