Adds support for signing in with multiple identities. Closes #3587#5790
Adds support for signing in with multiple identities. Closes #3587#5790martinlingstuyl wants to merge 7 commits intopnp:mainfrom
Conversation
martinlingstuyl
force-pushed
the
multiple-connections
branch
2 times, most recently
from
1ca38ec to
03a254e
Compare
|
Hey @martinlingstuyl, is this PR right? Seems like we've got some extra files modified that aren't a part of the proposed change. Could you please have another look? |
|
Can you name the ones you mean? I know I had issues with some tests, which caused me to do more changes than what seems necessary |
|
Also: do check out the PR description where I mention a few of those |
I'm seeing 777 files changed and included in the PR, a bunch of them are docs. Are all of those changes necessary? Also, it seems like we've got some conflicts. 😊 |
My bad, the files seem to be modified tests. All good. If you could please resolve conflicts, then I'll start the review. |
martinlingstuyl
force-pushed
the
multiple-connections
branch
2 times, most recently
from
823e33c to
54a27f3
Compare
|
done @waldekmastykarz ! |
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
Very nice! Let's do a couple of changes before we merge.
In general, let's pay attention to the following:
- let's use single quotes where possible. If we need to escape an apostrophe, let's use backticks
- let's use a single assert per test. Otherwise, if a test fails, the only way to find out what's wrong is to debug it, which isn't convenient
- let's avoid to use
!to suppress null validation. It can lead to runtime errors, which we could otherwise catch design time thanks to typings
martinlingstuyl
force-pushed
the
multiple-connections
branch
from
df194e6 to
e228d22
Compare
|
Hi @waldekmastykarz, it's kinda hard to find comment replies on GH is my experience, (when there are a lot of comments) if you reply to a resolved comment, could you unresolved it? |
| throw new CommandError(`The connection name '${newName}' is already in use`); | ||
| } | ||
|
|
||
| allConnections.forEach(c => { |
There was a problem hiding this comment.
To avoid iterating over all connections twice, let's update the code, so that we store the outcome of allConnections.filter in an array. We can then use it to compare its length first, and if we found only one object, directly update its name.
There was a problem hiding this comment.
I've changed it a bit, as there's only one entry of the connection available.
|
Were those all comments you had @waldekmastykarz ? Or is the review not done yet? |
This was it. You've done awesome job 👏 Let me have another look and we'll get it in for folks to test. |
|
I'll see if the conditionally nullable and assert does a better job instead of the null checks |
|
Ok @waldekmastykarz, I fixed your comments. I also added some |
|
Just to double check: is the PR ready for review? |
|
Yes it is! Unless I must first fix the merge conflicts 😀 |
martinlingstuyl
force-pushed
the
multiple-connections
branch
from
1635712 to
215e4e7
Compare
|
All yours @waldekmastykarz! |
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
I've just noticed an undesired side-effect: when you run m365 status and you're not signed in, you get a login prompt. Let's revert this behavior to what we have now (status shows the current state without automatically requiring you to login).
|
Yeah, that's definitely not as it should. I'll check it out and update it |
|
I cannot reproduce this behavior @waldekmastykarz, Can you consistently reproduce it in some way? |
Odd, I can't repro it indeed. All seems to be working just fine. I wonder if it's related to the fact, that I was signed in with the current code base and then pulled in your PR, built it and ran the status command. Either way, all seems good. I'll proceed with the review. Thanks for double checking and sorry for the trouble. |
|
Merged manually. Brilliant work! So cool to see this out there 👏 |
|
This is really a game changer indeed. Superb! |
2 participants
Closes #3587
🚀 There we go again! Adding multiple connection functionality 🚀
connectionNameto the output of commands likem365 statusandm365 login.connection useoutput is the same asm365 statusPoints of attention
We need to test if Managed Identity still works. That's the only one I haven't tested yet.
No graceful fallback I've changed the shape of the data that's saved to our token caches. As a result I've decided (with Waldek) to just use different filenames. So after installing everyone needs to run m365 login again.
Specifically I've added:
nameidentityNameidentityIdNotes for reviewers
I've overridden the base action on the
connection <verb>command. The reason is that we need to be able to execute these commands when we don't have an active connection. So for example: If I have two connections and I remove one (which is also my active connection) I'll be left in a semi-logged-out state. I need to be able to executeconnection useto switch to another connection, hence I'm overriding the base command action.There's some changes in test cases that seem to be unrelated to the PR. For example:
spo page add,spo page set,spo site ensure,todo task add,todo task set. These changes are due to issues that were invisible till now. (missing stubs that cause issues with authentication for example)