Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds 'spe container activate' command. Closes #6086 #6435

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nanddeepn
Copy link
Contributor

Adds spe container activate command. Closes #6086

@Jwaegebaert
Copy link
Contributor

Thanks @nanddeepn, we'll try to review it ASAP!

@Adam-it Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 29, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 29, 2024

@nanddeepn I added the hacktoberfest-accepted label to this PR which means that this PR will count as done for the Hacktoberfest event. So if you participate in this event it will get you unblocked and it allows us to merge this PR later when we catch up 👍
Thanks for your support and awesome contribution 👏 You Rock 🤩

@milanholemans
Copy link
Contributor

@Adam-it, I was looking at this PR and noticed that we are using two permission scopes in the entire spe group. For some commands we are using SharePoint CSOM APIs, for other commands, we are using Graph, some commands use both. Wouldn't it make sense that we need only 1 permission scope to run all spe commands? In this case, we could get rid of the Graph API request and perform a request to the SP REST API. This would apply also to other commands like spe container list.

@Adam-it
Copy link
Member

Adam-it commented Dec 19, 2024

@Adam-it, I was looking at this PR and noticed that we are using two permission scopes in the entire spe group. For some commands we are using SharePoint CSOM APIs, for other commands, we are using Graph, some commands use both. Wouldn't it make sense that we need only 1 permission scope to run all spe commands? In this case, we could get rid of the Graph API request and perform a request to the SP REST API. This would apply also to other commands like spe container list.

@milanholemans thanks for the feedback.
The problem is that for managing containertype when I were creating specs for those commands I did not have any success in managing them using REST API.
Since then I already finished my project that used SPE at work 😜 so I did not recheck that part for some time but back in that time I had to use SPO management shell commands to create container types which under the hood use CSOM. I did a small attempt to try to use REST when Mathijs suggested that but it did not work at all 😕. Maybe now those REST endpoints got fixed and work. I developed a project around SPE in April and that is when I created initial issues for the SPE command in this repo so it has been some time already 😮
For managing anything other than container types like containers or files in them we may use MS Graph endpoints and that work.

Back in those days the SPE VS Code extension was still being developed but last time I checked now it has a lot more functionalities I may try to research what this extension uses under the hood to create container types. Maybe the REST API endpoints were fixed for this and are now working 🤷. This all requires some research from my side 😉 I will try to take it for a spin in January 👍

@milanholemans
Copy link
Contributor

If we add ?whatif to the Graph request, we get the URL of the API that's being used by Graph. For this request, that's SharePoint v2.0 API. I'm wondering if we can make it work with that request.

@Adam-it
Copy link
Member

Adam-it commented Dec 19, 2024

If we add ?whatif to the Graph request, we get the URL of the API that's being used by Graph. For this request, that's SharePoint v2.0 API. I'm wondering if we can make it work with that request.

ok I think I get what you mean. You want us to only use the SP scopes for the Entra app right? I am not sure as this might be a bit hacky approach.
The recommended way from the MS docs is to use both for MS Graph

{
  "resourceAppId": "00000003-0000-0000-c000-000000000000",
  "resourceAccess": [
    {
      "id": "085ca537-6565-41c2-aca7-db852babc212",
      "type": "Scope"
    },
    {
      "id": "40dc41bc-0f7e-42ff-89bd-d9516947e474",
      "type": "Role"
    }
  ]
}

and for SPO

{
  "resourceAppId": "00000003-0000-0ff1-ce00-000000000000",
  "resourceAccess": [
    {
      "id": "4d114b1a-3649-4764-9dfb-be1e236ff371",
      "type": "Scope"
    },
    {
      "id": "19766c1b-905b-43af-8756-06526ab42875",
      "type": "Role"
    }
  ]
},

@milanholemans
Copy link
Contributor

ok I think I get what you mean. You want us to only use the SP scopes for the Entra app right? I am not sure as this might be a bit hacky approach.

Yeah, right now if you want to use spe commands you need a SharePoint scope and a Graph scope. It would be convenient if you only had to consent 1 scope.

@Adam-it
Copy link
Member

Adam-it commented Dec 19, 2024

ok I think I get what you mean. You want us to only use the SP scopes for the Entra app right? I am not sure as this might be a bit hacky approach.

Yeah, right now if you want to use spe commands you need a SharePoint scope and a Graph scope. It would be convenient if you only had to consent 1 scope.

well that is the recommended way. I would not hack around that

@milanholemans
Copy link
Contributor

milanholemans commented Dec 19, 2024

well that is the recommended way. I would not hack around that

Why is this considered 'hacking'? It's just using an SP REST API. Is creating a list item using SP REST considered 'hacking' because there is a Graph endpoint for that?

@Adam-it
Copy link
Member

Adam-it commented Dec 20, 2024

well that is the recommended way. I would not hack around that

Why is this considered 'hacking'? It's just using an SP REST API. Is creating a list item using SP REST considered 'hacking' because there is a Graph endpoint for that?

good question, I didn't check if for all SPE operations, we will have a SP REST API endpoint for that. Nevertheless from what I remember in order to manage container types, containers, files etc it is a mix of everything 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: m365 spe container activate
4 participants