cli-microsoft365 icon indicating copy to clipboard operation
cli-microsoft365 copied to clipboard

Published 20 hours ago • verified publisher icon pnp

Extending 'aad app add' command with admin consent process

Open michaelmaillot opened this issue 3 years ago • 11 comments

  • Extends aad app add command to support admin consent

Closes #2563

michaelmaillot avatar Jun 29 '22 15:06 michaelmaillot

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

martinlingstuyl avatar Jul 01 '22 11:07 martinlingstuyl

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.

martinlingstuyl avatar Jul 27 '22 11:07 martinlingstuyl

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.

michaelmaillot avatar Jul 27 '22 13:07 michaelmaillot

Looks much better like this 🍻

martinlingstuyl avatar Jul 27 '22 14:07 martinlingstuyl

@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 😊

Adam-it avatar Jul 28 '22 07:07 Adam-it

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 avatar Jul 28 '22 08:07 martinlingstuyl

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

Adam-it avatar Jul 28 '22 08:07 Adam-it

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 avatar Jul 28 '22 12:07 Adam-it

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

martinlingstuyl avatar Jul 28 '22 13:07 martinlingstuyl

Thanks for your additional feedback @martinlingstuyl!

I'll update my code accordingly.

michaelmaillot avatar Jul 29 '22 06:07 michaelmaillot

@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 avatar Aug 09 '22 14:08 Adam-it

recreated this change in separate PR #3578

Adam-it avatar Aug 11 '22 23:08 Adam-it