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

Adding Get-PnPPowerPlatformConnector #3487

Closed
wants to merge 13 commits into from

Conversation

siddharth-vaghasia
Copy link
Contributor

Type

[x] New Feature

What is in this Pull Request ?

Added new command to Get PowerPlatform Solutions. This command would help to get the PowerPlatform solution for the given enviorment.

{
WriteVerbose($"Retrieving all Connectors within environment '{environmentName}'");

var connectors = GraphHelper.GetResultCollectionAsync<Model.PowerPlatform.Environment.PowerPlatformConnector>(Connection, $"https://api.powerapps.com/providers/Microsoft.PowerApps/apis?api-version=2016-11-01&$filter=environment eq '{environmentName}' and isCustomApi eq 'True'", AccessToken).GetAwaiter().GetResult();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same check here as well for GCC clouds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

file commited by mistake..removed

WriteVerbose($"Using default environment as retrieved '{environmentName}'");
}

string accessTokenForGettingSolutions = TokenHandler.GetAccessTokenforPowerPlatformSolutions(this, Connection, dynamicsScopeUrl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use RequiredMinimalApiPermissions attribute instead of this ? Please check some graph cmdlets where we use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This token that we are getting would have a dynamic URL based on the Power Platform environment....so token has to be get for url like "https://orge12323.crm8.dynamics.com" and this will change - so this would not worked with RequiredMinimalPermissions? Let me know if you think there is any other way to get token

Comment on lines +22 to +41
string environmentName = null;
string dynamicsScopeUrl = null;
var environments = GraphHelper.GetResultCollectionAsync<Model.PowerPlatform.Environment.Environment>(Connection, "https://management.azure.com/providers/Microsoft.ProcessSimple/environments?api-version=2016-11-01", AccessToken).GetAwaiter().GetResult();
if (ParameterSpecified(nameof(Environment)))
{
environmentName = Environment.GetName().ToLower();
WriteVerbose($"Using environment as provided '{environmentName}'");
dynamicsScopeUrl = environments.FirstOrDefault(e => e.Properties.DisplayName.ToLower() == environmentName || e.Name.ToLower() == environmentName)?.Properties.LinkedEnvironmentMetadata.InstanceApiUrl;
}
else
{
environmentName = environments.FirstOrDefault(e => e.Properties.IsDefault.HasValue && e.Properties.IsDefault == true)?.Name;
dynamicsScopeUrl = environments.FirstOrDefault(e => e.Properties.IsDefault.HasValue && e.Properties.IsDefault == true)?.Properties.LinkedEnvironmentMetadata.InstanceApiUrl;
if (string.IsNullOrEmpty(environmentName))
{
throw new Exception($"No default environment found, please pass in a specific environment name using the {nameof(Environment)} parameter");
}

WriteVerbose($"Using default environment as retrieved '{environmentName}'");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this code to a utility ?
It could be something like:

var env = Utility.GetEnvironment

Move the logic there and keep the cmdlet code itself smaller

Copy link
Contributor Author

@siddharth-vaghasia siddharth-vaghasia Oct 13, 2023

Choose a reason for hiding this comment

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

I was looking at how can I move it - but If you think about it we would only be able to move line 24, because once we got environments. I need conditions to find the environment name - (given by the user) also the Environment instance URL...so would have to keep that logic in the command itself...otherwise I would have to create separate model class for Env with only two attributes name and instanceURL.

any other thoughts?

namespace PnP.PowerShell.Commands.PowerPlatform.Environment
{
[Cmdlet(VerbsCommon.Get, "PnPPowerPlatformConnectors")]
public class GetPowerPlatformConnectors : PnPAzureManagementApiCmdlet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would require this I think:

[RequiredMinimalApiPermissions("https://management.azure.com/.default")]

namespace PnP.PowerShell.Commands.PowerPlatform.Environment
{
[Cmdlet(VerbsCommon.Get, "PnPPowerPlatformSolution")]
public class GetPowerPlatformSolution: PnPAzureManagementApiCmdlet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here:

Would require this I think:

[RequiredMinimalApiPermissions("https://management.azure.com/.default")]

Copy link
Contributor Author

@siddharth-vaghasia siddharth-vaghasia Oct 13, 2023

Choose a reason for hiding this comment

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

For PowerPlatform related commands I don't think it would require...as not seeing in any other commands also.

@siddharth-vaghasia
Copy link
Contributor Author

hi @gautamdsheth - did you get a chance to review the updated comments?

@KoenZomers
Copy link
Collaborator

If I may anothe review comment as well, could you rename the cmdlet to Get-PnPPowerPlatformCustomConnector? It only returns the custom connectors, not the out of the box ones. I find the current name confusing in this sense.

@KoenZomers KoenZomers changed the title Added new command Added Get-PnPPowerPlatformConnector Nov 7, 2023
@KoenZomers KoenZomers changed the title Added Get-PnPPowerPlatformConnector Adding Get-PnPPowerPlatformConnector Nov 7, 2023
@siddharth-vaghasia
Copy link
Contributor Author

If I may anothe review comment as well, could you rename the cmdlet to Get-PnPPowerPlatformCustomConnector? It only returns the custom connectors, not the out of the box ones. I find the current name confusing in this sense.

This is very good observation, changed the cmdlet to have custom word in it...

@siddharth-vaghasia siddharth-vaghasia marked this pull request as draft January 9, 2024 09:15
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.

3 participants