pnp
Command update - spo tenant settings set
Type
- [ ] Bug Fix
- [x] New Feature
- [ ] Sample
Related Issues?
Closes #5034
What is in this Pull Request ?
Added around 33 new options/parameters to spo tenant settings set cmdlet as discussed in issue #5034.
Amazing work @ganesh-sanap 👏 Looking forward in using these new options (especially IsLoopEnabled 🤭). You rock 🤩
@ganesh-sanap not yet no. As you can see, we have quite some PRs to process at the moment and we only do this in our free time. Also, I haven't really had time for the past few days. We will try to process it ASAP.
Thank you for being so patient.
Hi @ganesh-sanap we had a busy month to release the new major version of CLI. Because this PR is quite large I wasn't able to include it in that release. Will try my best in the coming weeks to review it. Sorry for the delay.
Hi, @ganesh-sanap it's been a while since this PR was raised. The last couple of months were quite busy for me. During this time, some PRs were merged and changed stuff to CLI. Could you rebase with the latest main version and fix the few errors? This will speed up the review process which I'll try to complete in the coming weeks.
Thanks!
@milanholemans Can you help with the steps or commands for rebasing with latest main branch to my local branch (spo-tenant-settings-set-bulk) without loosing changes made to my local branch?
Hi @ganesh-sanap definitely!
The best way to do this is by syncing your fork on GitHub with the latest changes and pulling these changes locally on your main branch.
Then you switch back to your feature branch and:
# Rebase to main
git rebase main
# Fix merge conflicts in VS Code, when ready click 'Continue' in VS Code.
# When done, push to GitHub, force flag is important! Don't push using VS Code.
git push --force
You can add commits that are added later as usual.
Thank you @milanholemans! I have done all the changes and marked this PR for your review.
Thank you @ganesh-sanap! Will start reviewing in the coming days (looking at the size of the PR, it will take awhile).
@milanholemans Pushed commit with required changes, thanks for review!
@ganesh-sanap don't want to put any stress on you at all, but did you find some time to look at the comments I made? Maybe you have lost sight of this? I wouldn't want this valuable work to be lost.
Hi @ganesh-sanap did you lose track of this? Don't want to put any pressure just double-checking.
Hi @ganesh-sanap do you still want to work on this? No problem if you don't have enough time, we can always open it up for someone else.
@milanholemans sorry for delay. I am working on it. I have few questions about changes requested, will ask separately. Thanks!
@ganesh-sanap are you still willing to work on this? How do you see this issue? Is it too big/too much work? Is it unclear? Are you still interested in it? You are welcome to answer honestly so we can find a way out.
Hi @milanholemans, I have completed all PR review changes except restricting enum values for ~LinkRole options, currently working on it. Hopefully I will complete it by this week and send it again for your review.
Regarding adding new options (ExpireVersionsAfterDays, MajorVersionLimit, EnableAutoExpirationVersionTrim), I checked the logic in PnP PowerShell code and I think these options requires checking existing tenant setting values & adding some rules/errors based on it - this will take more time. Shall we do this in separate issue/PR? Thanks!
Regarding adding new options (ExpireVersionsAfterDays, MajorVersionLimit, EnableAutoExpirationVersionTrim), I checked the logic in PnP PowerShell code and I think these options requires checking existing tenant setting values & adding some rules/errors based on it - this will take more time. Shall we do this in separate issue/PR? Thanks!
Is it that complicated? Aren't these just number and boolean fields?
@milanholemans while passing the options (ExpireVersionsAfterDays and MajorVersionLimit) to the command, first we have to check the existing value of EnableAutoExpirationVersionTrim tenant property. We have to allow updating ExpireVersionsAfterDays and MajorVersionLimit properties only when EnableAutoExpirationVersionTrim property is set to false.
I think we will have to do the GET call to tenant settings for adding these type of validation rules before updating the property values using this command. So, this will not be a simple change like other number and boolean fields.
@milanholemans while passing the options (
ExpireVersionsAfterDaysandMajorVersionLimit) to the command, first we have to check the existing value ofEnableAutoExpirationVersionTrimtenant property. We have to allow updatingExpireVersionsAfterDaysandMajorVersionLimitproperties only whenEnableAutoExpirationVersionTrimproperty is set tofalse.
Technically speaking, we should indeed. But is this really limited by the API itself? Does it throw an error if you don't? This command is so gigantic that it's nearly impossible to build validations for every command. Also would it make it quite unclear in my opinion.
Hi @ganesh-sanap. I know this PR is taking a while, but I haven't forgotten about it. We're currently preparing a V8 release with CLI which consumed quite some time from my side. I will try to find some time to do another review for this big PR. Maybe in the meantime you could rebase with the latest main and resolve the merge conflict to make it go a little bit faster?