pnp
Refactor `spo page set` and `spo page add` to use util instead of calling other command, Closes #5300
Thanks for another awesome PR 💪👍. You are on 🔥 today. We will review it ASAP
@nicodecleyre also here some tests are failing and it does not seem like some workflow glitch 😉. could you recheck locally?
some tests are failing and it does not seem like some workflow glitch 😉. could you recheck locally?
I see.. But I don't quite understand, it doesn't show these errors locally.. Even did a cli config reset (you never know) followed by npm run clean, npm run build, npm run test
That's perfectly possible, the pipeline Node v20 is failing while you are using v18 locally.
That's perfectly possible, the pipeline Node
v20is failing while you are usingv18locally.
Thank you for the clarification @milanholemans, I tried testing it locally with node v20, but it still seems to run without errors:
Do you have any guidance how to solve this failing pr? I don't really understand why it fails in the pipeline, also the fact that one of the failing test is one I didn't touched
That's perfectly possible, the pipeline Node
v20is failing while you are usingv18locally.
Actually on the pipes it's failing always 😅 node 16, 18, 20 and win or Ubuntu. Seems strange. I will check it running our devcontainer and see locally. I hope I will manage to check this over tonight. If not I will try to add it for tomorrow 👍
Actually on the pipes it's failing always 😅 node 16, 18, 20 and win or Ubuntu. Seems strange. I will check it running our devcontainer and see locally. I hope I will manage to check this over tonight. If not I will try to add it for tomorrow 👍
Thank you for wanting to help 🙏. no rush here, feel free to test locally with you whenever you got time!
Hi @Adam-it, first of all, thank you very, very much for looking through this and pointing out the problem!! Really appreciate it! I've come a long way with your explanation. But it still goes wrong during the prebuild where I run into a 243
So i tried with npm i only and it seems to be complaining about permissions denied.. I'm totally not familiar with docker so do you have any idea what i'm doing wrong? Do i have to give permissions on a certain folder?
@nicodecleyre are you using the devcontainer from here -> https://github.com/pnp/cli-microsoft365/tree/main/.devcontainer are you using some external drive for your workspace?
@nicodecleyre are you using the devcontainer from here -> https://github.com/pnp/cli-microsoft365/tree/main/.devcontainer are you using some external drive for your workspace?
Jup, i'm using that one and not using an external drive. But... Was able to reproduce it using GitHub Codespaces and was able to write a fix for the test 🥳. Thank you for your help along the way, you rock 🤩
Seems we still faced a timeout 😆 can you pretty please rerun the test?
Jup, i'm using that one and not using an external drive. But... Was able to reproduce it using GitHub Codespaces and was able to write a fix for the test 🥳. Thank you for your help along the way, you rock 🤩
Awesome news! No need to thank for the support. I didn't do much + we are in this together 👍
Seems we still faced a timeout 😆 can you pretty please rerun the test?
sure
Thanks for the fixes. I will review it ASAP
setting to draft due to the following observations I will provide more guidance how we should proceed with this PR in near future
@nicodecleyre how are things going? I was thinking if we could refresh those PR's and try to proceed with this and #5299 PR as one go. Also refactoring the parent commands like spo listitemset as described in the issue last comment. What do you think? Do you need to have a check on this together? or you need any kind of other help to get going?
@nicodecleyre how are things going? I was thinking if we could refresh those PR's and try to proceed with this and #5299 PR as one go. Also refactoring the parent commands like
spo listitemsetas described in the issue last comment. What do you think? Do you need to have a check on this together? or you need any kind of other help to get going?
Hi @Adam-it. I'm good but suuuuuperbusy right now, a lot of things come together at once 😅. How are you? I'll make some time this month and look into what you're asking
Hi @Adam-it,
Did what was requested! Not gonna lie, this was a hard one and spended a lot of time on this one 😅. I don't know if this is the best approach to go for this big PR's for the refactoring, but that's just my opinion..
Hi @Adam-it,
Did what was requested! Not gonna lie, this was a hard one and spended a lot of time on this one 😅. I don't know if this is the best approach to go for this big PR's for the refactoring, but that's just my opinion..
Thanks @nicodecleyre for the awesome work and another holdup from our side 🙏. I will start reviewing this PR today You Rock 🤩
@pnp/cli-for-microsoft-365-maintainers I would appreciate second 👀on this 🙏. Lets be sure we catch most of the things so that we may align other 42 refactor to util PRs in similar way lets start the merging 🚀
@nicodecleyre also now I just noticed that for spo listitem set command I think we introduced some unneeded log somewhere 🤔
I don't seem to have it from main branch. Let's investigate if there isn't any not needed log somewhere
@waldekmastykarz would you be so kind and give it another check since many of the comments were from your side? thanks 👍
@waldekmastykarz would you be so kind and give it another check since many of the comments were from your side? thanks 👍
@waldekmastykarz did by any chance you had the time to take a look at this one? thanks
@waldekmastykarz would you be so kind and give it another check since many of the comments were from your side? thanks 👍
@waldekmastykarz did by any chance you had the time to take a look at this one? thanks
Not yet, thanks for the ping
All the request changes are resolved now, can you guys give it another check @Adam-it & @waldekmastykarz 🙏 ?
All the request changes are resolved now, can you guys give it another check @Adam-it & @waldekmastykarz 🙏 ?
Thanks @nicodecleyre for your awesome work and quick turn around. I will go over this command over the weekend. Sorry for the holdup but I am still catching up with everything after Christmas break 🙂
All the request changes are resolved now, can you guys give it another check @Adam-it & @waldekmastykarz 🙏 ?
Thanks @nicodecleyre for your awesome work and quick turn around. I will go over this command over the weekend. Sorry for the holdup but I am still catching up with everything after Christmas break 🙂
No problem, take you're time, we're not in a hurry here 😉
LGTM 👍 Sorry @nicodecleyre once again for yet again hold up. I have a few minor comments which I may resolve myself when merging tested locally and seems to be working properly 👍 You rock 🤩
No need to apologize. I appreciate every effort you and others put into reviewing these PRs, as we all do this in our spare time. 🙏
@waldekmastykarz since you had most of the comments in this PR I leave the stage to you 👍.
Hey @nicodecleyre, could you please resolve the conflict so that we won't lose any of your fixes? Then we'll have another look at this PR and get it in if all is good. Thank you!
Hi @waldekmastykarz, I have already resolved quite a few merge conflicts regarding this PR, which were not always that easy. The merge conflicts currently present are from a command present in this PR that uses a new use of executecommand from a PR that has been merged during time, which we are just trying to refactor in this PR, and that is a bit discouraging if I may be so honest... Can we use an approach to minimize the overhead here and finally ship this PR?