search

pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
882 stars 312 forks source link

Using util functions in favor of commands calling other commands to reuse code #4531

Open waldekmastykarz opened 1 year ago

waldekmastykarz commented 1 year ago

Like we explained in https://github.com/pnp/cli-microsoft365/discussions/4486, we're going to replace the pattern of commands calling other commands to centralizing the shared code in util functions and using utils where needed.

Following commands call other commands and need to be refactored:

milanholemans commented 1 year ago

That are actually less commands than I expected!

milanholemans commented 1 year ago

This reminds me of #3618 which we also have to create a bunch of issues for.

waldekmastykarz commented 1 year ago

That are actually less commands than I expected!

We haven't gone to extremes to retrieving sites and other basic things through other commands which is why the impact is limited. Once we clean this up and take a look at the different API calls that we run, we'd probably find more candidates for centralization and removing duplicate code.

nicodecleyre commented 1 year ago

It would be nice if we could get rid of commands calling other commands. Could I start working on some of these?

Jwaegebaert commented 1 year ago

Could I start working on some of these?

Of course you can! Be sure when you create an issue for them to mention this epic, that way the issue will be linked. Also, to quickly create an issue here you can click on convert to issue when hovering over the task:

image

martinlingstuyl commented 1 year ago

@nicodecleyre: also, lets define the interface of the util and how it works before you go hacking away. That saves us time afterwards on discussion and possibly rework.

So pick an issue, define how you want to solve it, and ask for opinions.

Clear like that?

nicodecleyre commented 1 year ago

Of course you can! Be sure when you create an issue for them to mention this epic, that way the issue will be linked. Also, to quickly create an issue here you can click on convert to issue when hovering over the task:

Thx for the tip @Jwaegebaert! But that's something that seems to be only for an author of an issue or a maintainer.

@nicodecleyre: also, lets define the interface of the util and how it works before you go hacking away. That saves us time afterwards on discussion and possibly rework.

So pick an issue, define how you want to solve it, and ask for opinions.

Clear like that?

Sure, let's take #5208 as an example then.

For me the specification are:

Acceptance Criteria can be:

Jwaegebaert commented 1 year ago

But that's something that seems to be only for an author of an issue or a maintainer.

Ow pardon, had no clue that was a limited feature

martinlingstuyl commented 1 year ago

Very nice list of requirements @nicodecleyre! Let's include that in the actual issue descriptions.

As for the utils, I have a more specific description in mind as well:

I think that would be the best way forward.

martinlingstuyl commented 1 year ago

Example for the app catalog:

In Jsdoc setup. Something like follows: 

/**
  * Gets app catalog from tenant
  * returns undefined if none is configured
  * writes to error stream if none is configured.
  */
public getTenantAppCatalog(logger: Logger):Promise<string | undefined>

This is what we would need to discuss.

nicodecleyre commented 1 year ago

Very good, I would like to suggest one more suggestion, the description of the parameters:

/**
  * Gets app catalog from tenant
  * returns undefined if none is configured
  * writes to error stream if none is configured.
  * @param logger The logger object uses for the logging.
  */
public getTenantAppCatalog(logger: Logger):Promise<string | undefined>

Regarding the issues, i'm not quite sure what you mean there but I would suggest that we focus on the command and refactor all the Cli.executeCommand functions and refactor them to util functions. So we isolate the refactoring per command to keep it as clear as possible.

martinlingstuyl commented 1 year ago

the description of the parameters

Exactly. Mine was just by example.

martinlingstuyl commented 1 year ago

So we isolate the refactoring per command to keep it as clear as possible.

That's okay as well. As long as we describe the necessary util(s) up front.

nicodecleyre commented 12 months ago

That's okay as well. As long as we describe the necessary util(s) up front.

Alright, here is an overview:

Command The command to refactor Util function
app get AadAppGetCommand aadApp.getAppById
app permission list appGetCommand aadApp.getAppById
pa app get paAppListCommand pa.getAppByDisplayName
planner roster member add AadUserGetCommand aadUser.getUserIdByUpn
pp aibuildermodel remove PpAiBuilderModelGetCommand powerPlatform.getAiBuilderModelByName
pp card clone PpCardGetCommand powerPlatform.getCardByName
pp card remove PpCardGetCommand powerPlatform.getCardByName
pp chatbot remove PpChatbotGetCommand powerPlatform.getChatbotByName
pp solution publish PpSolutionGetCommand powerPlatform.getSolutionByName
pp solution publisher remove PpSolutionPublisherGetCommand powerPlatform.getSolutionPublisherByName
pp solution remove PpSolutionGetCommand powerPlatform.getSolutionByName
spfx project permissions grant SpoServicePrincipalGrantAddCommand spo.servicePrincipalGrant
spo contenttype add SpoContentTypeGetCommand spo.getContentTypeByListIdAndId
spo.getContentTypeByListTitleAndId
spo.getContentTypeByListUrlAndId
spo.getContentTypeById
spo eventreceiver remove SpoEventReceiverRemoveCommand spo.getEventReceiverByListIdAndName
spo.getEventReceiverByListTitleAndName
spo.getEventReceiverByListUrlAndName
spo.getEventReceiverByName
spo file move SpoFileRemoveCommand spo.removeFile
spo file rename SpoFileRemoveCommand spo.removeFile
spo file retentionlabel ensure SpoListItemRetentionLabelEnsureCommand spo.ensureRetentionLabelFromListItem
spo file retentionlabel remove SpoListItemRetentionLabelRemoveCommand spo.removeRetentionLabelFromListItem
spo file roleassignment add SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
SpoRoleDefinitionListCommand spo.getRoleDefinitionByName
SpoFileGetCommand spo.getFileById
spo file roleassignment remove SpoFileGetCommand spo.getFileById
SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
spo file roleinheritance break SpoFileGetCommand spo.getFileById
spo file roleinheritance reset SpoFileGetCommand spo.getFileById
spo file sharinglink clear spoFileSharingLinkListCommand spo.getFileSharingLinks
spo folder retentionlabel ensure SpoListItemRetentionLabelEnsureCommand spo.ensureListItemRetentionLabel
SpoListRetentionLabelEnsureCommand spo.ensureListRetentionLabel
spo folder retentionlabel remove SpoListItemRetentionLabelRemoveCommand spo.removeListItemRetentionLabel
SpoListRetentionLabelRemoveCommand spo.removeListRetentionLabel
spo folder roleassignment add SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
SpoRoleDefinitionFolderCommand spo.getRoleDefintionByName
spo folder roleassignment remove SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
spo group member remove AadUserGetCommandOptions aadUser.getUpnByUserEmail
SpoGroupMemberListCommand spo.getGroupMembersByGroupId
spo.getGroupMembersByGroupName
spo group set AadUserGetCommandOptions aadUser.getUpnByUserEmail
spo hubsite get SpoListItemListCommand spo.getListItems
spo list roleassignment add SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
SpoRoleDefinitionListCommand spo.getRoleDefinitionByName
spo list roleassignment remove SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
spo listitem retentionlabel ensure SpoWebRetentionLabelListCommand spo.getRetentionLabelByName
spo.getRetentionLabelById
spo listitem roleassignment add SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
SpoRoleDefinitionListCommand spo.getRoleDefinitionByName
spo listitem roleassignment remove SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
spo page add spoFileGetCommand spo.getFileAsListItemByUrl
spoListItemSetCommand spo.setListItem
spo page set spoFileGetCommand spo.getFileAsListItemByUrl
spoListItemSetCommand spo.setListItem
spo serviceprincipal permissionrequest approve SpoServicePrincipalPermissionRequestListCommand spo.listServicePrincipalPermissionRequests
spo site ensure spoSiteAddCommand spo.addSite
spoSiteSetCommand spo.setSite
spo site set aadO365GroupSetCommand aadGroup.setGroup
spoSiteDesignApplyCommand spo.applySiteDesign
spo tenant appcatalog add spoSiteAddCommand spo.addSite
spoSiteGetCommand spo.getSite
spoSiteRemoveCommand spo.removeSite
spoTenantAppCatalogUrlGetCommand spo.getTenantAppCatalogUrl
spo tenant applicationcustomizer add spoTenantAppCatalogUrlGetCommand spo.getTenantAppCatalogUrl
spoListItemAddCommand spo.addListItem
spoListItemListCommand spo.getListItems
spo tenant commandset add spoTenantAppCatalogUrlGetCommand spo.getTenantAppCatalogUrl
spoListItemAddCommand spo.addListItem
spoListItemListCommand spo.getListItems
spo web roleassignment add SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
SpoRoleDefinitionListCommand spo.getRoleDefinitionByName
spo web roleassignment remove SpoUserGetCommand spo.getUserByEmail
SpoGroupGetCommand spo.getGroupByName
teams app install AadUserGetCommand aadUser.getUpnByUserId
teams meeting attendancereport list AadUserGetCommand aadUser.getUserIdByUpn
aadUser.getUserIdByEmail
teams meeting get AadUserGetCommand aadUser.getUserIdByUpn
aadUser.getUserIdByEmail
teams meeting list AadUserGetCommand aadUser.getUserIdByEmail
teams team app list TeamGetCommand teams.getTeamByName
viva connections app create spoWebGetCommand spo.getWeb
nicodecleyre commented 12 months ago

For the sake of completeness, the following 2 commands have already been refactored to util functions, so they may be removed from the list of this epic:

But following commands are missing from the list of this epic

I had some time this weekend and refactored the commands in this epic 😮‍💨. I will wait on submitting the PRs, waiting for your feedback on the above overview and adjust the refactoring where necessary before submitting. In addition, I will also triple check whether everything is still working properly before submitting the PRs 🙏.

martinlingstuyl commented 12 months ago

Hi @nicodecleyre, I like your enthousiasm. 😁 However: I do want to pull on the brakes for a minute.

What I meant by describing the utils upfront was the following:

This would not need to be very long. but it gives us the possibility and time to decide/discuss the implementation details. It also means that you won't spend valuable time going down a path and having to refactor everything afterwards.

Now that you already started it's maybe the easiest if you create issues for each command that you refactored. Take care to copy/write down the util interface and implementation details, so we can discuss it in the issue specs (instead of in the PR, which is harder for discussions) You can connect your PR for that command at the same time.

Is that okay for you?

nicodecleyre commented 11 months ago

That's ok for me. Thank you @martinlingstuyl!

nicodecleyre commented 11 months ago

Apologies for the numerous PRs I've submitted 😭; I realize it might be overwhelming. I'm aware that you all dedicate your free time to incredible work on the CLI, so I completely understand if these PRs aren't reviewed immediately. They don't introduce any breaking changes, just some nice-to-have improvements, so feel free to prioritize other PRs with more urgency. Thank you all for your efforts in maintaining the cli!

I put in my best effort to minimize the workload for you all and thoroughly tested each command before creating the PRs. However, during testing, I noticed that the following commands no longer functioned correctly:

I will fix and rebase merge them hopefully somewhere next week.

Adam-it commented 11 months ago

Apologies for the numerous PRs I've submitted 😭; I realize it might be overwhelming. I'm aware that you all dedicate your free time to incredible work on the CLI, so I completely understand if these PRs aren't reviewed immediately. They don't introduce any breaking changes, just some nice-to-have improvements, so feel free to prioritize other PRs with more urgency. Thank you all for your efforts in maintaining the cli!

I put in my best effort to minimize the workload for you all and thoroughly tested each command before creating the PRs. However, during testing, I noticed that the following commands no longer functioned correctly:

  • spo site ensure
  • spo site set
  • spo tenant appcatalog add
  • spo tenant applicationcustomizer add
  • spo tenant commandset add

I will fix and rebase merge them hopefully somewhere next week.

Awesome work 👍. No need to apologize IMO 🙂. At least we won't get bored 🤣. Thanks for the double check. Could you please mark the related PRs with failing commands as draft?

nicodecleyre commented 11 months ago

Thanks for the double check. Could you please mark the related PRs with failing commands as draft?

i haven't uploaded them yet, will do once fixed 😄

martinlingstuyl commented 11 months ago

Great work @nicodecleyre, due to the holiday time I don't have much time to look at it. But I will eventually 😀

Adam-it commented 9 months ago

ok after some time I think we may get back to this issue and try to clear out the clutter a bit on the PR list. @nicodecleyre internally we agreed that:

  1. we won't be pushing those PRs for v7 but rather wait a bit and start merging this after the major release
  2. after rechecking the opened PRs we have noticed our previous decisions were not the best and we should rethink and refactor the approach 🤦. As there are many PRs opened connected to this issue we suggest setting all of those as draft and start rebasing/refactoring and then merging them into smaller groups (single PRs). The main issue with the currently developed approach is:
    • currently, we have many PRs introducing the same util so merging a single PR will create conflicts in others. This is a bit problematic. As a result, it would be cleaner to merge some PRs into single (bigger) PRs and introduce the same change once.
    • looking at the PRs and let's consider spo page set and spo page add as an example, we noticed that the util functions introduced in those PRs are:
      • quite big and specialized. Therefore it is quite hard to call them util. Maybe those should be then split into smaller once so it will make them easier to reuse
      • we noticed now we introduced a lot of duplicated code for example the spo util setListItem is in many ways very similar or even the same (line by line) as the command itself, so in this case, spo listitem set. Due to that we should now also refactor the spo listitem set at the same time. Leaving this as is will lead to having the same code in two places which makes it harder to maintain, with reusing the command we did not have it. We should fix it up at once (single go) as if we open a separate issue for it, it's hard to predict how long we will have such a duplicated state.

due to the above, we suggest to:

waldekmastykarz commented 9 months ago

I'd like to propose an alternative approach. Big PRs are hard to review and it costs a lot of time to get them right, leading to additional reviews, adjustments and merge conflicts.

What if instead we break it into smaller steps and don't try to do everything at once. What I mean: say we've got a piece of code that would become a util and is used in 4 commands. Why not start with just one, extract the util, merge the PR and then refactor other commands one by one? It would simplify the review and isolate the work so that it doesn't depend on other things, and also is not affected by other changes that we've got in parallel.

Adam-it commented 9 months ago

@waldekmastykarz if it's one util and 4 commands then we are more or less looking at a PR that will update 9 files (1 for the util, 4 files are the commands and 4 files are the tests for those commands). It does not seem that big.

Also if we split this into smaller steps like:

  1. PR for the util
  2. PR for command 1
  3. PR for command 2
  4. PR for command 3
  5. PR for command 4

and let's say only open and merge for PRs for step 1,2,3. Then let's say something happens, priority change and we are left with work for step 4 and 5 and also potentially we are left with some duplicated code.

This is what we may have even now if we just refactor a single command and create util and not refactor the 'parent' commands.

I agree that opening large PRs is hard to maintain but from what I suggested to group the current PRs into a bit bigger once we should not end up with PRs that update more than 20 files. At least that is what I expect. What do you think? Seems legit 😉

waldekmastykarz commented 9 months ago

Our goal is to process PRs as quickly as possible. I'd avoid settling on arbitrary number of files like 10 or 20, because it doesn't express the amount of changes that are being submitted. The more changes PR authors submit at once, the longer it takes to review the PR and the bigger the odds for a conflict that the PR author will need to resolve before we can review the PR. Having more granular changes allows us to move faster, and we should work with our contributors so that they won't open too many PRs at once which again will take a long time to review and has a risk of conflicts.

nicodecleyre commented 9 months ago

I'm sorry I've created this many PR's at once.

But I don't really get it, what are we trying to discuss here? I thought everything was ok, we even had acceptance criteria and now we want to change things after the PR's are submitted?

Merge conflicts will occur anyway, I've already put a lot of time in solving merge conflicts in every PR while the CLl moved to ESM. I hope not but it would be a shame if the PR's or better said the time I put into them is worthless.

Adam-it commented 9 months ago

But I don't really get it, what are we trying to discuss here? I thought everything was ok, we even had acceptance criteria and now we want to change things after the PR's are submitted?

Thats true. But even though it was well prepared 👏 we noticed some things only after we started reviewing the PR's. This things happen and it is sometimes impossible to anticipate all things before the actual work (or part of work) is done.

Merge conflicts will occur anyway, I've already put a lot of time in solving merge conflicts in every PR while the CLl moved to ESM. I hope not but it would be a shame if the PR's or better said the time I put into them is worthless.

For sure not. We understand it is a lot of effort and time spent for which we are truly grateful and also will do our very best so that it won't go to waste 👍 I think all in all we are on the right track but:

  1. for sure it will take some time to process so please consider that probably we will be merging this for the next couple of months probably 😉.
  2. before we proceed we need to amend a few details. (more below)

please double check my comment above https://github.com/pnp/cli-microsoft365/issues/4531#issuecomment-1722583117 It may bring some more light to this discussion. I think the conflicts we will get when merging are actually not that important so even we may leave the PRs as are now (as @waldekmastykarz suggested) and not group them into bigger once as I suggested 👍. The main point we should amend is

looking at the PRs and let's consider spo page set and spo page add as an example, we noticed that the util functions introduced in those PRs are:

  • quite big and specialized. Therefore it is quite hard to call them util. Maybe those should be then split into smaller once so it will make them easier to reuse
  • we noticed now we introduced a lot of duplicated code for example the spo util setListItem is in many ways very similar or even the same (line by line) as the command itself, so in this case, spo listitem set. Due to that we should now also refactor the spo listitem set at the same time. Leaving this as is will lead to having the same code in two places which makes it harder to maintain, with reusing the command we did not have it. We should fix it up at once (single go) as if we open a separate issue for it, it's hard to predict how long we will have such a duplicated state.

So it's important (....and this is something we only noticed just now, sorry for that) that we not only refactor the commands to use util instead of other commands (let's call them 'parent' commands) but also avoid code duplication. So if we created an util based on the 'parent' command logic we should also refactor the 'parent' command to use those utils so that we avoid the same code in two separate files (places). As I investigated this it will be not that easy as the created utils are quite large and specialized but it 'should' (famus last words) be possible 😉. @nicodecleyre let me know what you think of the above argument and feel about it.

if you agree I suggest also that we go step by step. So let's focus on PRs that introduced similar util (like spo page set and spo page add is a great example) lets for now only try to merge those two (leaving others in draft) so we will see if there is anything else we will find/learn along the way. Those PRs are in good shape, the only problem they have is the duplicated code like the one from spo listitem set, if we may sort it out in one of those PRs then we should be ready to go 🙂

nicodecleyre commented 2 months ago

Hi everyone!

As #5301 has been merged, do you think we can open the next one? What would be the next steps here?

Adam-it commented 2 months ago

Hi everyone!

As #5301 has been merged, do you think we can open the next one? What would be the next steps here?

hi @nicodecleyre, thanks for the reminder and the answer is yes, totally! Sorry for the hold up as this is a bit on me now. I wanted to review the rest of your PRs now and check which now we may combine as they propose similar change and which 'parent' commands should also be refactored to use utils. If you have any suggestions they are more than welcome 👍

nicodecleyre commented 2 months ago

Hi everyone! As #5301 has been merged, do you think we can open the next one? What would be the next steps here?

hi @nicodecleyre, thanks for the reminder and the answer is yes, totally! Sorry for the hold up as this is a bit on me now. I wanted to review the rest of your PRs now and check which now we may combine as they propose similar change and which 'parent' commands should also be refactored to use utils. If you have any suggestions they are more than welcome 👍

Hi Adam! Thank you for your response. I would suggest that we stay in the spo range and maybe go for spo file?

Adam-it commented 2 months ago

@nicodecleyre what if we merge those 4 PRs into a single https://github.com/pnp/cli-microsoft365/pull/5269 https://github.com/pnp/cli-microsoft365/pull/5271 https://github.com/pnp/cli-microsoft365/pull/5273 https://github.com/pnp/cli-microsoft365/pull/5275

those create a common util method so it makes a lot of sense to be single PR and as I checked there is no duplication in the logic we have in the spo file get so it seems quite clean to just merge the 4 PRs into single new PR and merge this in a single go. What do you think?

nicodecleyre commented 2 months ago

@nicodecleyre what if we merge those 4 PRs into a single https://github.com/pnp/cli-microsoft365/pull/5269 https://github.com/pnp/cli-microsoft365/pull/5271 https://github.com/pnp/cli-microsoft365/pull/5273 https://github.com/pnp/cli-microsoft365/pull/5275

those create a common util method so it makes a lot of sense to be single PR and as I checked there is no duplication in the logic we have in the spo file get so it seems quite clean to just merge the 4 PRs into single new PR and merge this in a single go. What do you think?

Sounds good to me 👍 will create a PR in the coming days