Fast as always, 👍 we'll review as soon as we can. Though due to some vacations here, that might take a little while 😬

Fast as always, 👍 we'll review as soon as we can. Though due to some vacations here, that might take a little while 😬

All good, I'm not in a hurry. Also vacations are more important 🙂

Great, thanks for the quick turn around. Will review/merge it today night 👍.
You rock 🤩🤩

I pulled and started testing it locally and I noticed just one strange behavior for the --onlyAllowMembersViewMembership. This is the only option when not specified is true by default. I understand this is the OOO behavior but I was wondering wouldn't it make more sense and be more aligned with what we have in the spec that when we don't define it it will be false. Now it seems this is the only one that we need to explicitly set to false.

❯ m365 spo group add --webUrl https://tenanttocheck.sharepoint.com/sites/hr-life --name "test3"
{
  "Id": 20,
  "IsHiddenInUI": false,
  "LoginName": "test3",
  "Title": "test3",
  "PrincipalType": 8,
  "AllowMembersEditMembership": false,
  "AllowRequestToJoinLeave": false,
  "AutoAcceptRequestToJoinLeave": false,
  "Description": null,
  "OnlyAllowMembersViewMembership": true,
  "OwnerTitle": "Adam Wójcik",
  "RequestToJoinLeaveEmailSetting": null
}

❯ m365 spo group add --webUrl https://tenanttocheck.sharepoint.com/sites/hr-life --name "test4" --onlyAllowMembersViewMembership false
{
  "Id": 21,
  "IsHiddenInUI": false,
  "LoginName": "test4",
  "Title": "test4",
  "PrincipalType": 8,
  "AllowMembersEditMembership": false,
  "AllowRequestToJoinLeave": false,
  "AutoAcceptRequestToJoinLeave": false,
  "Description": null,
  "OnlyAllowMembersViewMembership": false,
  "OwnerTitle": "Adam Wójcik",
  "RequestToJoinLeaveEmailSetting": null
}

Let me know what do you think? For me seems a bit unexpected behavior when compered to other options 🤔 Sorry for the late comment I know you were expecting this to be already ready to merge but I would like it to be perfect 👍. Lets discuss this one last thing and we are ready to go 😉 You are doing a awesome job 🤩

Strange, I hadn't noticed this one yet. I will check this one tomorrow, command has to be perfect indeed. 😃

Strange, I hadn't noticed this one yet. I will check this one tomorrow, command has to be perfect indeed. 😃

let me know if you need any help. I will be awaiting your feedback on that but there is no rush 👍👍. You rock 🤩

I understand this is the OOO behavior but I was wondering wouldn't it make more sense and be more aligned with what we have in the spec that when we don't define it it will be false. Now it seems this is the only one that we need to explicitly set to false.

In my opinion, this might be a bit tricky. When we create a group via the UI, OnlyAllowMembersViewMembership is also true. If we're going to put this on false by default, means that when you create a group via the UI or via CLI it will give a different result. Personally I prefer to get rid of the switches and implement the options as true or false. This way it works the same as the spo group set command.
What do you think?

In my opinion, this might be a bit tricky. When we create a group via the UI, OnlyAllowMembersViewMembership is also true. If we're going to put this on false by default, means that when you create a group via the UI or via CLI it will give a different result. Personally I prefer to get rid of the switches and implement the options as true or false. This way it works the same as the spo group set command. What do you think?

Yes we may do that and which will align it with the group set command.
Easier approach (if this is the same behavior as SP has when working from UI) would be just to leave it and add this behavior in the Remarks section in docs 🤔
@pnp/cli-for-microsoft-365-maintainers any better ideas/comments?

Yes we may do that and which will align it with the group set command.
Easier approach (if this is the same behavior as SP has when working from UI) would be just to leave it and add this behavior in the Remarks section in docs 🤔
@pnp/cli-for-microsoft-365-maintainers any better ideas/comments?

If I'm not mistaken, if we leave it as is, you are unable to set it to false. Because the switch value can be either true or undefined.

Yes we may do that and which will align it with the group set command.
Easier approach (if this is the same behavior as SP has when working from UI) would be just to leave it and add this behavior in the Remarks section in docs 🤔
@pnp/cli-for-microsoft-365-maintainers any better ideas/comments?

If I'm not mistaken, if we leave it as is, you are unable to set it to false. Because the switch value can be either true or undefined.

It is possible. Using flag option with false value works. please double check to code snippet I posted earlier

It is possible. Using flag option with false value works. please double check to code snippet I posted earlier

Aw yeah I missed that 2nd example. In that case there are 2 options indeed. Leave it as is, or refactor it to true, false options.

Good catch @Adam-it!

Using a flag for OnlyAllowMembersViewMembership in this case would not make sense, even if you can add the false value. So I think it's better to change this one to a normal boolean option. With a remark in the docs that true is the default.

I'm a little doubtful at what to do with the other options. On the one hand it would seem logical to just change this one only, as it's then clear something specific is going on with it. But on the other hand that also seems rather inconsistent in this case. As all these options look alike and kind of belong together.

I think I'd prefer moving them all to boolean options. What about you?

I think I'd prefer moving them all to boolean options. What about you?

This one gets my vote ☺️

I think I'd prefer moving them all to boolean options. What about you?

This one gets my vote ☺️

+1 here as well 😉

In that case: let's follow that route!

FWIW, if we expect people to specify a value, let's make it clear in the docs and option's spec. If we specify an option as a flag, it should be used as such. I think there's a pattern for negating flags, like --no-flagName but it's not a widely-known thing, so to keep the command's usage obvious, let's define it as a regular option that accepts true|false as values.

merged manually