Refactor `spo page set` and `spo page add` to use util instead of calling other command, Closes #5300 #5301

nicodecleyre · 2023-07-18T14:35:33Z

Adam-it · 2023-07-18T14:36:41Z

Thanks for another awesome PR 💪👍. You are on 🔥 today. We will review it ASAP

Adam-it · 2023-07-18T18:29:12Z

@nicodecleyre also here some tests are failing and it does not seem like some workflow glitch 😉.
could you recheck locally?

nicodecleyre · 2023-07-19T07:29:51Z

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

milanholemans · 2023-07-19T11:54:52Z

That's perfectly possible, the pipeline Node v20 is failing while you are using v18 locally.

nicodecleyre · 2023-07-19T12:26:15Z

That's perfectly possible, the pipeline Node v20 is failing while you are using v18 locally.

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

Adam-it · 2023-07-19T14:33:32Z

Will check this one as well. I think it is related to the other PR

Adam-it · 2023-07-19T14:40:32Z

That's perfectly possible, the pipeline Node v20 is failing while you are using v18 locally.

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 👍

nicodecleyre · 2023-07-19T14:43:12Z

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!

Adam-it

@nicodecleyre I managed to reproduce the issues from the pipeline locally and unfortunately, those are connected to small mistakes done in code. I pointed out one mistake done in the review comments.

The problem shows up when running this in a container on ubuntu. You may check it yourself locally. With the devcontainer we have in the repo it is quite easy, just install Docker for windows, install the Remote Containers VS Code extension and run the Reopen in Container and that's it 👍. You may follow those details in the wiki for more details
https://github.com/pnp/cli-microsoft365/wiki/GitHub-Codespaces-&-Visual-Studio-Remote-Development-Container#local-remote-development-container

Please try to recheck it locally.

src/utils/spo.spec.ts

nicodecleyre · 2023-07-31T13:44:34Z

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?

Adam-it · 2023-07-31T18:49:33Z

@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 · 2023-07-31T18:54:04Z

@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?

Adam-it · 2023-07-31T18:57:02Z

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

Adam-it · 2023-09-17T22:28:48Z

setting to draft due to the following observations
I will provide more guidance how we should proceed with this PR in near future

Adam-it

@nicodecleyre this is a good start 👍. Let's recheck if we may improve this a bit and also
this should be merged together with #5299 as a single change in order to avoid code conflicts

src/utils/spo.ts

Adam-it · 2023-10-09T08:56:44Z

@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 · 2023-10-10T19:01:43Z

@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?

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

nicodecleyre · 2023-10-31T16:54:39Z

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..

Adam-it · 2023-11-18T13:35:48Z

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 🤩

Adam-it

@nicodecleyre awesome work. Seems a lot better and I love the fact we are moving more to utils also in the co-related commands and as a result we now have less code (instead of duplicated logic).
I think we are on the right track till now I only found minor details we may improve along the way 👏👍

I noticed that when using spo page set I am unable to, but maybe it is also like this from main branch 😅 We could recheck 👍

src/utils/spo.ts

src/m365/spo/commands/page/page-add.ts

Adam-it · 2023-11-18T14:34:17Z

@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 🚀

src/utils/spo.ts

waldekmastykarz · 2024-02-10T09:17:17Z

src/utils/spo.ts

+    }
+
+    const parsedUrl = new URL(requestUrl);
+    const serverRelativeSiteUrl = requestUrl.match(new RegExp('/sites/[A-Za-z0-9-]*'))![0];


This comment seems to be marked as resolved without any comment or changes. Why is that?

src/utils/spo.ts

waldekmastykarz · 2024-03-23T09:49:42Z

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!

nicodecleyre · 2024-03-23T10:02:34Z

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?

waldekmastykarz · 2024-03-23T10:18:08Z

Sorry for the trouble, and that's totally on us. We'll resolve the conflict ourselves.

nicodecleyre · 2024-03-23T10:47:40Z

Sorry for the trouble, and that's totally on us. We'll resolve the conflict ourselves.

Don't get me wrong, I'm totally fine into resolving these merge conflicts myself 😅. I have time tomorrow anyway. I just hope we can prioritize the refactoring prs (this one and the ones I had to close) a bit (bearing in mind that we all do this in our spare time) so we can minimize the overhead and time they are open 🙏

waldekmastykarz · 2024-04-13T07:26:32Z

Merged manually. Thank you! Once again, sorry it's taken us so long.

milanholemans marked this pull request as draft July 18, 2023 19:42

Adam-it requested changes Jul 19, 2023

View reviewed changes

src/utils/spo.spec.ts Outdated Show resolved Hide resolved

Adam-it self-assigned this Jul 21, 2023

nicodecleyre marked this pull request as ready for review July 31, 2023 18:54

nicodecleyre force-pushed the Refactor-spo-page-set branch from 08e2a6e to cdb87ea Compare September 6, 2023 09:04

Adam-it marked this pull request as draft September 17, 2023 22:28

Adam-it requested changes Sep 17, 2023

View reviewed changes

src/utils/spo.ts Outdated Show resolved Hide resolved

src/utils/spo.ts Outdated Show resolved Hide resolved

Adam-it mentioned this pull request Sep 17, 2023

Refactor spo page add to use util instead of calling other command, Closes #5298 #5299

Closed

nicodecleyre force-pushed the Refactor-spo-page-set branch from cdb87ea to b159b8f Compare October 25, 2023 13:25

nicodecleyre marked this pull request as ready for review October 31, 2023 16:54

Adam-it changed the title ~~Refactor spo page set to use util instead of calling other command, Closes #5300~~ Refactor spo page set and spo page add to use util instead of calling other command, Closes #5300 Nov 7, 2023

Adam-it requested changes Nov 18, 2023

View reviewed changes

src/utils/spo.ts Outdated Show resolved Hide resolved

src/utils/spo.ts Outdated Show resolved Hide resolved

src/utils/spo.ts Outdated Show resolved Hide resolved

src/utils/spo.ts Outdated Show resolved Hide resolved

src/m365/spo/commands/page/page-add.ts Outdated Show resolved Hide resolved

waldekmastykarz requested changes Feb 10, 2024

View reviewed changes

waldekmastykarz marked this pull request as draft February 10, 2024 09:23

nicodecleyre force-pushed the Refactor-spo-page-set branch from 3adebfd to 8f7fa8e Compare February 24, 2024 17:05

nicodecleyre marked this pull request as ready for review February 24, 2024 17:43

waldekmastykarz marked this pull request as draft March 23, 2024 09:49

waldekmastykarz marked this pull request as ready for review March 23, 2024 10:18

nicodecleyre added 12 commits March 24, 2024 14:22

First commit

ea90666

Fix after testing + added logging

d881928

Fix

8ecf436

Woot new errors.... I don't get this

8a5bd1f

Right.... That explains

d267737

Merge with spo page add + missing test in spo file get

2b6518f

Changes

43db974

Changes from Adam's review

d11ba67

Waldek changes

3b34f3a

changes

c7c2b30

Changes

ecc8eaf

Changes

3bbf2a2

nicodecleyre force-pushed the Refactor-spo-page-set branch from a73cea1 to 3bbf2a2 Compare March 24, 2024 13:33

Changes after rebase

2879950

waldekmastykarz approved these changes Apr 13, 2024

View reviewed changes

waldekmastykarz added the pr-merged label Apr 13, 2024

waldekmastykarz closed this Apr 13, 2024

nicodecleyre mentioned this pull request Apr 19, 2024

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

Open

53 tasks

nicodecleyre mentioned this pull request Apr 29, 2024

Refactor spo page add to use util instead of calling other command #5298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `spo page set` and `spo page add` to use util instead of calling other command, Closes #5300 #5301

Refactor `spo page set` and `spo page add` to use util instead of calling other command, Closes #5300 #5301

nicodecleyre commented Jul 18, 2023

Adam-it commented Jul 18, 2023

Adam-it commented Jul 18, 2023

nicodecleyre commented Jul 19, 2023 •

edited

Loading

milanholemans commented Jul 19, 2023 •

edited

Loading

nicodecleyre commented Jul 19, 2023

Adam-it commented Jul 19, 2023

Adam-it commented Jul 19, 2023

nicodecleyre commented Jul 19, 2023

Adam-it left a comment

nicodecleyre commented Jul 31, 2023

Adam-it commented Jul 31, 2023

nicodecleyre commented Jul 31, 2023

Adam-it commented Jul 31, 2023

Adam-it commented Sep 17, 2023

Adam-it left a comment

Adam-it commented Oct 9, 2023

nicodecleyre commented Oct 10, 2023

nicodecleyre commented Oct 31, 2023

Adam-it commented Nov 18, 2023

Adam-it left a comment

Adam-it commented Nov 18, 2023

waldekmastykarz Feb 10, 2024

waldekmastykarz commented Mar 23, 2024

nicodecleyre commented Mar 23, 2024

waldekmastykarz commented Mar 23, 2024

nicodecleyre commented Mar 23, 2024

waldekmastykarz commented Apr 13, 2024

Refactor spo page set and spo page add to use util instead of calling other command, Closes #5300 #5301

Refactor spo page set and spo page add to use util instead of calling other command, Closes #5300 #5301

Conversation

nicodecleyre commented Jul 18, 2023

Adam-it commented Jul 18, 2023

Adam-it commented Jul 18, 2023

nicodecleyre commented Jul 19, 2023 • edited Loading

milanholemans commented Jul 19, 2023 • edited Loading

nicodecleyre commented Jul 19, 2023

Adam-it commented Jul 19, 2023

Adam-it commented Jul 19, 2023

nicodecleyre commented Jul 19, 2023

Adam-it left a comment

Choose a reason for hiding this comment

nicodecleyre commented Jul 31, 2023

Adam-it commented Jul 31, 2023

nicodecleyre commented Jul 31, 2023

Adam-it commented Jul 31, 2023

Adam-it commented Sep 17, 2023

Adam-it left a comment

Choose a reason for hiding this comment

Adam-it commented Oct 9, 2023

nicodecleyre commented Oct 10, 2023

nicodecleyre commented Oct 31, 2023

Adam-it commented Nov 18, 2023

Adam-it left a comment

Choose a reason for hiding this comment

Adam-it commented Nov 18, 2023

waldekmastykarz Feb 10, 2024

Choose a reason for hiding this comment

waldekmastykarz commented Mar 23, 2024

nicodecleyre commented Mar 23, 2024

waldekmastykarz commented Mar 23, 2024

nicodecleyre commented Mar 23, 2024

waldekmastykarz commented Apr 13, 2024

Refactor `spo page set` and `spo page add` to use util instead of calling other command, Closes #5300 #5301

Refactor `spo page set` and `spo page add` to use util instead of calling other command, Closes #5300 #5301

nicodecleyre commented Jul 19, 2023 •

edited

Loading

milanholemans commented Jul 19, 2023 •

edited

Loading