Skip to content

Extending 'aad app add' command with admin consent process#3455

Closed
michaelmaillot wants to merge 9 commits intopnp:mainfrom
michaelmaillot:feat/aad-app-adminconsent
Closed

Extending 'aad app add' command with admin consent process#3455
michaelmaillot wants to merge 9 commits intopnp:mainfrom
michaelmaillot:feat/aad-app-adminconsent

Conversation

@michaelmaillot
Copy link
Copy Markdown
Contributor

@michaelmaillot michaelmaillot commented Jun 29, 2022

  • Extends aad app add command to support admin consent

Closes #2563

@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Jul 1, 2022

Thanks for your contribution @michaelmaillot 👏, we'll review this asap!

@Adam-it Adam-it self-assigned this Jul 5, 2022
Adam-it
Adam-it requested changes Jul 5, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@michaelmaillot thanks for your awesome work 👍 your rock 🤩
could you take a look at the comments I left 😉

Comment thread docs/docs/cmd/aad/app/app-add.md
Comment thread src/m365/aad/commands/app/app-add.ts Outdated
Comment thread src/m365/aad/commands/app/app-add.ts Outdated
Comment thread src/m365/aad/commands/app/app-add.ts
@Adam-it Adam-it removed their assignment Jul 5, 2022
@Adam-it Adam-it marked this pull request as draft July 5, 2022 22:44
@Adam-it Adam-it self-assigned this Jul 26, 2022
@michaelmaillot michaelmaillot marked this pull request as ready for review July 27, 2022 06:24
@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Jul 27, 2022

Hi @michaelmaillot, I'm seeing a lot of files changed. It seems to me that can't possibly be correct. I didn't look at the branch line as I'm on holiday, but I think something needs to be done here before we can review.

@michaelmaillot michaelmaillot force-pushed the feat/aad-app-adminconsent branch from 77f5e41 to 9095d5c Compare July 27, 2022 13:01
@michaelmaillot
Copy link
Copy Markdown
Contributor Author

michaelmaillot commented Jul 27, 2022 •
edited
Loading

Is this because I've synced my fork and rebase to main before pushing?

Sorry I've missed something but can't find out what, since it's a forked repo.

Edit : I think I've found what went wrong. I commit my changes before doing a pull --rebase, ending in being ahead of main, because of branch conflict. I've rebased locally then force pushed changes. Seems fine now, sorry for the inconvenience.

@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Jul 27, 2022

Looks much better like this 🍻

Adam-it
Adam-it reviewed Jul 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

pulled and tested locally and works great ✔
code seems ok ✔
one small mistake in the docs but please leave it, I will fix it with a quick commit when merging this one 😉
@michaelmaillot good job 👍 You Rock 🤩

Comment thread docs/docs/cmd/aad/app/app-add.md Outdated
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Jul 28, 2022

@martinlingstuyl do you think you could do an additional review on this one 🙏
from me it's ready to be merged (with a small fix along the way I will do) but I would like to double check this one with you if you don't mind 😊

@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Jul 28, 2022

Hi @Adam-it, I can review the code, but I do not have a pc with me (on holiday) so I can't try it out. I assume that's okay, as you have confirmed it works as expected. I can see if I can find some things just by looking at it line by line. That okay for you?

@martinlingstuyl martinlingstuyl self-assigned this Jul 28, 2022
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Jul 28, 2022

Hi @Adam-it, I can review the code, but I do not have a pc with me (on holiday) so I can't try it out. I assume that's okay, as you have confirmed it works as expected. I can see if I can find some things just by looking at it line by line. That okay for you?

totally

martinlingstuyl
martinlingstuyl requested changes Jul 28, 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 @michaelmaillot, @Adam-it, I've left some comments and considerations.👍

Comment thread src/m365/aad/commands/app/app-add.spec.ts Outdated
Comment thread src/m365/aad/commands/app/app-add.ts
Comment thread src/m365/aad/commands/app/app-add.ts Outdated
Comment thread src/m365/aad/commands/app/app-add.ts Outdated
Comment thread src/m365/aad/commands/app/app-add.ts Outdated
Comment thread src/m365/aad/commands/app/app-add.ts Outdated
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Jul 28, 2022

Hi @michaelmaillot, @Adam-it, I've left some comments and considerations.👍

great 🤩 Thanks @martinlingstuyl for the double check 👍👍👍. I didn't feel confident enough in merging this 😊 and I suppose the things you pointed out are good comments
@michaelmaillot do you think you could have a look at them ?

@Adam-it Adam-it marked this pull request as draft July 28, 2022 12:28
@martinlingstuyl
Copy link
Copy Markdown
Contributor

martinlingstuyl commented Jul 28, 2022

👍 It's all from my phone from holidays @michaelmaillot, some things might be easily answered and resolved. 😌

@michaelmaillot
Copy link
Copy Markdown
Contributor Author

michaelmaillot commented Jul 29, 2022

Thanks for your additional feedback @martinlingstuyl!

I'll update my code accordingly.

@martinlingstuyl martinlingstuyl removed their assignment Jul 29, 2022
@michaelmaillot michaelmaillot mentioned this pull request Aug 3, 2022
Closed
@michaelmaillot michaelmaillot force-pushed the feat/aad-app-adminconsent branch from 9095d5c to bd950c0 Compare August 6, 2022 22:33
@michaelmaillot michaelmaillot marked this pull request as ready for review August 6, 2022 22:39
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Aug 9, 2022

@michaelmaillot thanks for you amazing work 🤩. I will try to review it asap and align to our latest approach with commands which was refactored 👍

@Adam-it Adam-it mentioned this pull request Aug 11, 2022
Closed
@Adam-it
Copy link
Copy Markdown
Member

Adam-it commented Aug 11, 2022

recreated this change in separate PR #3578

@Adam-it Adam-it closed this Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinlingstuyl martinlingstuyl martinlingstuyl requested changes

@Adam-it Adam-it Adam-it requested changes

Assignees

@Adam-it Adam-it

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extend 'aad app add' to support granting admin consent

4 participants

@michaelmaillot @martinlingstuyl @Adam-it @waldekmastykarz