Enhancement: Allow unknown options for entra group/app/administrativeunit commands. Closes #6314 #6543
Conversation
|
Thanks, we'll try to look at it ASAP. |
src/utils/entraApp.ts
Outdated
| options: AppCreationOptions, | ||
| apis: RequiredResourceAccess[], | ||
| logger: Logger, | ||
| verbose: boolean, | ||
| debug: boolean | ||
| debug: boolean, | ||
| command: Command |
There was a problem hiding this comment.
I'm unsure what to think of this... I understand we need to call the addUnknownOptionsToPayload method which is on the command class. But still, it seems a bit odd to have to pass around the entire command as a parameter. Added to that we have to pass debug and verbose as parameters as well, which are on the command class as protected properties, so you can't call them from here unless you pass them separately.
There was a problem hiding this comment.
I see you also need to make addUnknownOptionsToPayload public instead of private for this to work.
Maybe as an alternative we could pass a delegate function into this util instead. The delegate in its stead calls the addUnknownOptionsToPayload from the command itself.
What do you think @pnp/cli-for-microsoft-365-maintainers?
There was a problem hiding this comment.
The current solution is definitely not the best. I wanted to avoid major refactoring...
There was a problem hiding this comment.
Yea I understand, which is why I think a delegate function parameter (or callback function) is better, but let's see what the other maintainers think...
There was a problem hiding this comment.
Passing the whole command as an arg is definitely not the way to go. I agree that we should make addUnknownOptionsToPayload more accessible from utils. Since it will become pretty generic (basically merging two objects), we need to consider a refactoring. No easy way out here unfortunately. Good call @martinlingstuyl 👏
There was a problem hiding this comment.
Moving to the optionsUtil should be possible.
There was a problem hiding this comment.
It seems to me that the callback function is causing tests failure for macOs (https://github.com/pnp/cli-microsoft365/actions/runs/13099770947/job/36546562680) when the npm run lint is executed.
Same for the optionsUtils.
There was a problem hiding this comment.
The addUnknownOptionsToPayload and getUnknownOptions moved to the optionsUtils.
There was a problem hiding this comment.
You're being a bit fast 😂 we needed to discuss and decide first. but let's see what you built, can you push the commit? It's okay if the tests aren't finished...
That way we can see if this is indeed the direction to go
There was a problem hiding this comment.
@martinlingstuyl TBH the current approach (I think the latest) seems clear and fine for me. I don't see any risks with this approach I guess.
@MartinM85 the error you mentioned on MacOs is actually connected to out of memory issue on GH workflow.
I don't expect it to be because of your current development. I think we already noticed it also randomly failing in other places as well
src/utils/entraApp.ts
Outdated
| options: AppCreationOptions, | ||
| apis: RequiredResourceAccess[], | ||
| logger: Logger, | ||
| verbose: boolean, | ||
| debug: boolean | ||
| debug: boolean, | ||
| command: Command |
There was a problem hiding this comment.
I see you also need to make addUnknownOptionsToPayload public instead of private for this to work.
Maybe as an alternative we could pass a delegate function into this util instead. The delegate in its stead calls the addUnknownOptionsToPayload from the command itself.
What do you think @pnp/cli-for-microsoft-365-maintainers?
MartinM85
force-pushed
the
feature/6314-allow-unknown-options
branch
from
cc64776 to
5997c49
Compare
Closes #6314