Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added M365 connections to shared #2090
base: main
Are you sure you want to change the base?
Added M365 connections to shared #2090
Changes from 10 commits
8c42981
c649f72
4d11e96
94c8466
263fdc3
a01e021
00d7a60
638d5c4
c4aea20
07362dd
34d9c86
c888a58
b9e8291
e7b64d1
129ee01
2199981
b6685e5
665eee0
63c065c
11011db
d4e7f88
9954010
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we returning a connection here? Trying to understand this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big reason why is trying to understand this section of code:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to return the connection because you could use different connections and you can control which one is under your responsibility, which one you created and which one is not your connection.
On that part of the code, I want to create a connection only if you do not have a connection and you do not use allow multiplesessions.
In case that you use allowmultiplesession, we will verify that we do not have a duplicated prefix.
It could be useful in future in case that we want to close a created connection or the one that you are going to use.
For example, in MDO we get information from EXO connection and compare with Graph connection to confirm that we are connected to the same tenant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this correctly work when you have multiple sessions used with
$AllowMultipleSessions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, you are sending by parameter the session that you created, just one, and that is the one that we are showing.
If you create a second one or a third one, is going to have a new call to the function that shows that information.
I think I test it when I created, but I will test it and paste some screenshots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay,
$newConnection
should only have 1 object in the array by the time we get here, so it should work as expected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a parameter validation here for if multiple
$Modules
are provided, that we should not be trying to find a$MinModuleVersion
available.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting I did not see that documented.
Maybe could be better to check one by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also address this prior to merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't required if we are setting
$confirmed
prior to testing it in anif
statement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could leave, or this an option as well:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not break the flow in any function, I return null and leave the decision to the one that use the function to stop if they do not get what they expect.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could change to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think we should
exit
if we find an error. If we are using this function, we should just straight upexit
if we run into an issue as the request of the script shouldn't work. Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the first comment, agree, it is easier. I will simplify it.
About second one, similar to previous one, if works I return true if fails I return false and leave the decision on the one that use the function.
I think maybe you can try to request another version and it is more flexible.
What do you think?