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

feat: ✨ add new command 'm365 pp pipeline list' to list Powe… #6434

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DevPio
Copy link

@DevPio DevPio commented Oct 17, 2024

Closes #6287

@milanholemans
Copy link
Contributor

Thank you @DevPio m, we'll try to review it ASAP!

@Adam-it Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 29, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 29, 2024

@DevPio I added the hacktoberfest-accepted label to this PR which means that this PR will count as done for the Hacktoberfest event. So if you participate in this event it will get you unblocked and it allows us to merge this PR later when we catch up 👍
Thanks for your support and awesome contribution 👏 You Rock 🤩

@martinlingstuyl martinlingstuyl self-assigned this Jan 2, 2025
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @DevPio, I've reviewed your PR, let's fix some issues before we move on!

```sh
m365 pp pipeline list [options]
```
`-e, --environment <environment>`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something got deleted here. We're missing an Options header and some characters.

@@ -31,5 +31,6 @@ export default {
SOLUTION_PUBLISHER_LIST: `${prefix} solution publisher list`,
SOLUTION_PUBLISHER_REMOVE: `${prefix} solution publisher remove`,
TENANT_SETTINGS_LIST: `${prefix} tenant settings list`,
TENANT_SETTINGS_SET: `${prefix} tenant settings set`
TENANT_SETTINGS_SET: `${prefix} tenant settings set`,
PIPELINE_LIST: `${prefix} pipeline list`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this in the alphabetically sorted correct place

@@ -1,5 +1,6 @@
import type { SidebarsConfig } from '@docusaurus/plugin-content-docs';

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you added a white line here by accident. Let's remove it.

@@ -1786,6 +1787,15 @@ const sidebars: SidebarsConfig = {
}
]
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put the pipeline section in the alphabetically sorted location (before s)

import GlobalOptions from '../../../../GlobalOptions.js';
import request, { CliRequestOptions } from '../../../../request.js';
import { formatting } from '../../../../utils/formatting.js';

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this white line

public async commandAction(logger: Logger, args: any): Promise<void> {

try {
const environmentDetails = await this.getEnvironmentDetails(args.options.environmentName, args.options.asAdmin);
Copy link
Contributor

Choose a reason for hiding this comment

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

All other pp commands use an existing piece of code to retrieve the instanceUrl. Can't we just use that here instead of coding a new function?

      const dynamicsApiUrl = await powerPlatform.getDynamicsInstanceApiUrl(args.options.environmentName, args.options.asAdmin);

catch (ex: any) {
this.handleRejectedODataJsonPromise(ex);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove superfluous line breaks

responseType: 'json'
};
const pipelines = await request.get<any>(pipelineListRequestOptions);
return pipelines.value.map((p: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use white lines in select places for readability:

  • Before a return statement
  • Before and after new 'scopes', like if-statements, loops, functions, etc.

const pipelines = await request.get<any>(pipelineListRequestOptions);
return pipelines.value.map((p: any) => {

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of limiting the number of properties we return, let's just return the entire api output.

The defaultproperties section will influence what columns will be visible in text outputs. But in Json output we'll be able to view it all, which is probably better.

import { sinonUtil } from '../../../../utils/sinonUtil.js';
import { accessToken } from '../../../../utils/accessToken.js';
import { CommandError } from "../../../../Command.js";

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove superfluous white lines

@martinlingstuyl martinlingstuyl marked this pull request as draft January 8, 2025 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: m365 pp pipeline list
4 participants