cli-microsoft365 icon indicating copy to clipboard operation
cli-microsoft365 copied to clipboard

Enhancement: Use the Microsoft Graph Types package in our commands

Open plamber opened this issue 3 years ago • 14 comments

Based on the discussion #2427 we would like to introduce a new package @microsoft/microsoft-graph-types to our project to have strongly typed results when calling Microsoft Graph.

This should reduce the likelihood to introduce bugs based on typing errors.

The aim of this issue is to go through all commands and replace the "custom" types with the "official" types provided by the package.

For the beta endpoints we should consider the "@microsoft/microsoft-graph-types-beta" package.

Here are the commands that need to be updated:

aad -> O365Group

  • [ ] o365group-get
  • [ ] o365group-list
  • [ ] o365group-set
  • [ ] o365group-add
  • [ ] o365group-recyclebinitem-clear
  • [ ] o365group-recyclebinitem-list
  • [ ] o365group-recyclebinitem-set

aad -> user

  • [ ] o365group-user-list
  • [ ] o365group-user-set
  • [ ] user-list

plamber avatar May 15 '21 11:05 plamber

@pnp/cli-for-microsoft-365-maintainers do we still see here some benefits or should we close this issue due to lack of interest?

plamber avatar Nov 07 '21 07:11 plamber

I'd be interested in keeping this open, maybe we should consider doing some investigation to create a list of commands that currently use custom types that can be replaced with the official types?

Once we have a list of commands, this will help us better understand the scope of this change.

garrytrinder avatar Nov 07 '21 15:11 garrytrinder

Instead of going through lists I could just start searching a command and refactor it with types.

I could give it a shot

plamber avatar Nov 07 '21 16:11 plamber

All yours @plamber 👍

garrytrinder avatar Nov 08 '21 20:11 garrytrinder

We use the package already so it would be a matter of identifying custom types and replacing them with a reference to the type from the package. Even if we use beta APIs, we should see if we need access to beta properties or if we can use the information from the main package to avoid having to reference two of them.

waldekmastykarz avatar Nov 09 '21 16:11 waldekmastykarz

Had a look into our commands.

  • Almost all team commands implement a dedicated interface in our cli. These can be replaced with the graph types
  • Something similar happens for the users and groups endpoints
  • Same under rstieclassification

There are also other places but these are the first ones that I found by performing a simple scan.

Should we create dedicated issues for each namespace or can we create multiple pull requests and associate them with this issue?

Cheers

plamber avatar Nov 14 '21 10:11 plamber

Ideally, let's create a checklist of all affected commands and tests that need updating so that we can track progress and know when we're done. Once we have it, we could create multiple PRs linked to the same issue.

waldekmastykarz avatar Nov 15 '21 08:11 waldekmastykarz

aad -> O365Group

  • [ ] o365group-get
  • [ ] o365group-list
  • [ ] o365group-set
  • [ ] o365group-add
  • [ ] o365group-recyclebinitem-clear
  • [ ] o365group-recyclebinitem-list
  • [ ] o365group-recyclebinitem-set

aad -> user

  • [ ] o365group-user-list
  • [ ] o365group-user-set
  • [ ] user-list

plamber avatar Dec 26 '21 05:12 plamber

I moved the list of commands to the issue for readability

waldekmastykarz avatar Dec 28 '21 10:12 waldekmastykarz

Looking at the project, there are other custom types beyond just Group and User that we've added so let's complete the list of affected commands. #2902 only completes this issue partially.

waldekmastykarz avatar Dec 31 '21 13:12 waldekmastykarz

What shall we do with this one @plamber, @waldekmastykarz, move it back to help wanted?

martinlingstuyl avatar Aug 28 '22 11:08 martinlingstuyl

Yes, good catch @martinlingstuyl! Let's remove custom interfaces where possible.

waldekmastykarz avatar Aug 28 '22 17:08 waldekmastykarz

Ideally, let's create a checklist of all affected commands and tests that need updating so that we can track progress and know when we're done. Once we have it, we could create multiple PRs linked to the same issue.

Should we turn this into an epic and convert each checklist item into an issue for each command, this way we keep one PR to one issue?

garrytrinder avatar Aug 31 '22 11:08 garrytrinder

I don't think we should do it per-command, because if you remove the interface, it will break multiple commands. Instead, we should make the list based on the interfaces that we use, so that each PR will contain the removed interface and one or more adjusted commands.

waldekmastykarz avatar Sep 02 '22 06:09 waldekmastykarz

I could give this a go :-)

MathijsVerbeeck avatar Dec 26 '22 22:12 MathijsVerbeeck

Great to hear @MathijsVerbeeck! All yours!

milanholemans avatar Dec 26 '22 22:12 milanholemans