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

Various fixes when using specific Connections #1919

Merged
merged 9 commits into from
Jun 3, 2022

Conversation

KoenZomers
Copy link
Collaborator

@KoenZomers KoenZomers commented May 31, 2022

Type

  • Bug Fix
  • New Feature
  • Sample

Related Issues?

Fixes #1857
Fixes #1729
Fixes #1724

What is in this Pull Request ?

Lots of cleanup and refactoring around using a specific connection retrieved by Connect-PnPOnline -ReturnConnection or Get-PnPConnection. Things were getting completely mixed up. It should now completely be separated from the current context which even would allow having multiple connections at once, each with a different authentication method or user.

@heinrich-ulbricht
Copy link

I so hope that this is the solution for #1949

@KoenZomers Is this already going to the prerelease builds?

@KoenZomers
Copy link
Collaborator Author

@heinrich-ulbricht, this is already in the latest nightly builds, yes. I did find a regression with this PR that prevents one from using disconnect-pnponline and then connect-pnponline again after that. I know where the issue lies, just need to discuss within the team what makes most sense in how to fix it.

@heinrich-ulbricht
Copy link

@KoenZomers Ok I'll have to wait for that because I use Disconnect-PnPOnline followed by Connect-PnPOnline (i.e. connect for all connections, I ran into #1857 as well).

@KoenZomers
Copy link
Collaborator Author

Currently discussing this in our team. What makes most sense to you in this scenario: you disconnect your session using Disconnect-PnPOnline so you could connect to another site collection. In the same PowerShell session, you use Connect-PnPOnline to connect to the other site collection. Would you be expecting:

  1. To authenticate again since you're connecting again
  2. To not have to authenticate again since you authenticated to that tenant before already, even though you did a disconnect-pnponline

Curious to hear what most people find logical here.

@heinrich-ulbricht
Copy link

heinrich-ulbricht commented Jun 8, 2022

@KoenZomers I would totally expect that I would have to authenticate again. Like the first time. Disconnect is disconnect. Otherwise I wouldn't be doing a disconnect but just using another connection from another connect, storing the previous connection for re-use.

I also have this scenario: get a couple of sites from SharePoint search (like 100-ish), iterate them, connect to each of them, do work, disconnect. Never going to use those connections again. My assumption would also be that a disconnect saves resources.

(Maybe) another argument: I use disconnect to protect me from doing errors. Disconnect everything to make sure I don't accidentally re-use a connection. Whether this is an argument depends on how re-using connections in your second variant looks like.

BUT I could live with either variant - as long as it's stable and allows working reliably with multiple connections 🤗

@KoenZomers
Copy link
Collaborator Author

Thanks for your extensive input. I agree that it would be most intuitive to work this way. Problem we're facing is that it hasn't worked this way and even was deliberately coded not to work this way, so to do automatically authenticate when connecting to the same tenant again. Thereby changing this would/could be seen as a non-backwards compatible change, which is something we really try to stay away from with all the scripts people have running unattended. Tricky one. We'll give it some more thoughts what's wise here.

@Nathaire
Copy link

Nathaire commented Jul 5, 2022

I think this PR create a breaking change in the flow. We had a script using $myconnec = Connect-PnPOnline -ReturnConnection and then later Disconnect-PnPOnline -Connection $myconnec

Before we get a warning, but now it throws an exception
image

Should breaking change like this not only be integrated in major version change, not minor one ?

@KoenZomers
Copy link
Collaborator Author

@Nathaire This is an unintended regression. Will look into fixing this.

@Nathaire
Copy link

Nathaire commented Jul 5, 2022

@KoenZomers thanks a lot

@KoenZomers
Copy link
Collaborator Author

The disconnect situation will be fixed by this PR: #2093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants