Skip to content

Conversation

@merill
Copy link
Contributor

@merill merill commented Jun 20, 2023

Fixes #

Changes proposed in this pull request

  • Fixed non-working, autogenerated example and added a new one

Other links

@merill merill requested a review from a team as a code owner June 20, 2023 10:04
Copy link
Contributor

@timayabi2020 timayabi2020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@merill thanks for noticing this issue. Here are my suggestions for the example to work as expected.

  1. Roll back to the previous example 1 title.
  2. Since you are targeting to merge into "dev" branch which is responsible for V1 releases, we don't need "-MgBeta" prefix on cmdlets. Cmdlets with that prefix are only available on V2 releases and the branch responsible is "features/2.0".So remove this New-MgBetaEntitlementManagementConnectedOrganization -DomainName microsoft.com.
  3. Update $params with below.
$params = @{
	displayName = "Connected organization name"
	description = "Connected organization description"
	identitySources = @(
		@{
			"@odata.type" = "#microsoft.graph.domainIdentitySource"
			domainName = "example.com"
			displayName = "example.com"
		}
	)
	state = "proposed"
}

Check Api reference here.
4. Delete the second example
5. I have confirmed that the above request body works
image

@merill
Copy link
Contributor Author

merill commented Jun 20, 2023

@timayabi2020 Using body parameters is not really the best way to do things in PowerShell. Also the other parameters like the name are optional. Do we really need to have them?

Also, is there a reason why we want to remove the second example? I can fix the name of the cmdlet to work with Graph PS v1 beta

@timayabi2020
Copy link
Contributor

@merill I agree indeed that the use of body parameters might not be ideal, however the main reason as to why we should still maintain it, is because the example makes use of the -BodyParameter documented on the PowerShell reference docs. It's also in-line with the API reference docs which I had earlier shared.
I therefore propose we do the following:

  1. Update the first example with the changes I had proposed.
  2. Update the title of the first example to indicate/show the use of body parameter.
  3. Have New-MgEntitlementManagementConnectedOrganization -DomainName microsoft.com as the second example. with its corresponding title.
  4. Keep the last example with IdentitySources parameter.

I will also open an issue on the PowerShell reference documentation repo to have the "DomainName" parameter documented.

@merill
Copy link
Contributor Author

merill commented Jun 22, 2023

I have put back the body parameter example and also fleshed out the examples. Cheers.

@timayabi2020 timayabi2020 merged commit 8d1fae9 into microsoftgraph:dev Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants