Skip to content

Table list command. Solves #3653#3684

Closed
appieschot wants to merge 9 commits intopnp:mainfrom
appieschot:feature/cmd-3653
Closed

Table list command. Solves #3653#3684
appieschot wants to merge 9 commits intopnp:mainfrom
appieschot:feature/cmd-3653

Conversation

@appieschot
Copy link
Copy Markdown
Member

@appieschot appieschot commented Sep 22, 2022

How to test

Create a new Azure AD App registration that you can use with the CLI for Microsoft. Provide this app the at least:

  • Azure Service Management - user_impersonation (to access Power Apps API)
  • Dynamics CRM - user_impersonation (to access Dynamics)
  • SharePoint - AllSites.FullControl to read tenant admin site

Use this new App Registration to sign in to the CLI m365 login --appId yourAppId --tenant yourTenantId

Then you should be able to run this command :)

⚠️ Alert

This can only go live with support from @waldekmastykarz as he can change our App Registration to include the Dynamics CRM - user_impersonation permission that is currently not yet available in the CLI App Registration

@appieschot appieschot marked this pull request as draft September 22, 2022 13:45
@appieschot
Copy link
Copy Markdown
Member Author

appieschot commented Sep 22, 2022

@pnp/cli-for-microsoft-365-maintainers Can I get a few insights before we merge this PR, the current approach is a method in a new Dynamics Base class. Check one do we feel this should be a method like this, or should we go more for AppCommand.ts base where we took a different approach? Or do I need to take a different approach at all with testing? Let me know what you think!

@waldekmastykarz
Copy link
Copy Markdown
Member

waldekmastykarz commented Sep 23, 2022

@appieschot we should consider moving the Dynamics instance URL retrieval to a util (we don't have one for PP so we'd create it). That way the method will be available to all the code in the CLI instead of only Dataverse commands.

@appieschot appieschot marked this pull request as ready for review September 27, 2022 15:52
@appieschot
Copy link
Copy Markdown
Member Author

appieschot commented Sep 27, 2022

Should be good now, tests working so ready for review. Please do see the first message around permissions :)

@martinlingstuyl martinlingstuyl self-assigned this Oct 4, 2022
martinlingstuyl
martinlingstuyl requested changes Oct 4, 2022
View reviewed changes
Copy link
Copy Markdown
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 Albert, I added some minor comments, but nothing serious. Great work, almost there! 🤙

Comment thread docs/docs/cmd/pp/dataverse/dataverse-table-list.md Outdated
Comment thread docs/docs/cmd/pp/dataverse/dataverse-table-list.md Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.ts Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.ts Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.ts Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.spec.ts Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.spec.ts Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.spec.ts Outdated
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.spec.ts
Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.spec.ts
@martinlingstuyl martinlingstuyl marked this pull request as draft October 4, 2022 20:04
@appieschot appieschot marked this pull request as ready for review October 5, 2022 08:49
@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Oct 5, 2022

Hi @appieschot, the build is failing since your latest changes. Could you please look into that?

@appieschot appieschot marked this pull request as draft October 5, 2022 11:30
@appieschot
Copy link
Copy Markdown
Member Author

appieschot commented Oct 5, 2022

@martinlingstuyl and here I was promised that as maintainer we would fix the barrel fix that was implemented by Waldek ;-). But I will give it a shot

@appieschot
Copy link
Copy Markdown
Member Author

appieschot commented Oct 5, 2022

Mmm, guess my rebase game is not that strong as I now have all commits in main in this one as well. @martinlingstuyl would you mind a short call to discuss how to fix this ;)? Build is fixed but I am wondering if a new PR is quicker compared to trying to cherry pick the right commits

@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Oct 5, 2022

Mmm, guess my rebase game is not that strong as I now have all commits in main in this one as well. @martinlingstuyl would you mind a short call to discuss how to fix this ;)? Build is fixed but I am wondering if a new PR is quicker compared to trying to cherry pick the right commits

Are you live on teams?

@Jwaegebaert Jwaegebaert added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 6, 2022
@appieschot appieschot marked this pull request as ready for review October 7, 2022 07:06
This was referenced Oct 7, 2022
Closed
Closed
Closed
martinlingstuyl
martinlingstuyl requested changes Oct 7, 2022
View reviewed changes
Copy link
Copy Markdown
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.

We're almost there. One last comment 🤙

Comment thread src/m365/pp/commands/dataverse/dataverse-table-list.ts
@martinlingstuyl martinlingstuyl marked this pull request as draft October 7, 2022 13:10
@appieschot appieschot mentioned this pull request Oct 7, 2022
Closed
@waldekmastykarz
Copy link
Copy Markdown
Member

waldekmastykarz commented Oct 8, 2022

Dynamics permissions added to our AAD app reg @appieschot

@appieschot
Copy link
Copy Markdown
Member Author

appieschot commented Oct 8, 2022

All eyes on @martinlingstuyl to make sure we tackle / agree on the last comment 🚀

@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Oct 8, 2022

You mean the try catch in the command class. That's correct indeed. In that case it's fine as it is! I'll approve it and merge it Monday.

@martinlingstuyl martinlingstuyl marked this pull request as ready for review October 8, 2022 15:45
@appieschot appieschot mentioned this pull request Oct 10, 2022
Closed
@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Oct 10, 2022

⚡Merged manually, thank you 🤙

@martinlingstuyl martinlingstuyl modified the milestone: v5.9 Oct 10, 2022
@appieschot appieschot mentioned this pull request Oct 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinlingstuyl martinlingstuyl martinlingstuyl approved these changes

Assignees

@martinlingstuyl martinlingstuyl

Labels

hacktoberfest-accepted Accept for hacktoberfest, will merge later pr-merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@appieschot @waldekmastykarz @martinlingstuyl @Jwaegebaert