Skip to content

[WIP] Migrate 'entra administrativeunit remove' to Zod #6813

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Jul 25, 2025

Thanks for assigning this issue to me. I'm starting to work on it and will keep this PR's description up to date as I form a plan and make progress.

Original issue description:

Migrate 'entra administrativeunit remove' to Zod.

Use the migrate to zod prompt file.

Fixes #6812.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@martinlingstuyl
Copy link
Contributor

I see you don't have access to the git commits that are referenced in the prompt file. I told you to stop working if that happens. Try again and don't use shallow clone to make sure you can get the commit details.

@martinlingstuyl
Copy link
Contributor

I see you don't have access to the git commits that are referenced in the migrate-command-to-zod.prompt.md file. I told you to stop working if that happens. Try again drafting a PR, making sure you don't use shallow clone so you can get the commit details.

Co-authored-by: martinlingstuyl <5267487+martinlingstuyl@users.noreply.github.com>
Copy link
Author

Copilot AI commented Jul 25, 2025

I see you don't have access to the git commits that are referenced in the migrate-command-to-zod.prompt.md file. I told you to stop working if that happens. Try again drafting a PR, making sure you ...

I've successfully accessed the reference commits after fetching the full git history. The migration has been completed using the patterns from commit 824c1eb. Changes committed in 779f911.

Copilot finished work on behalf of martinlingstuyl July 25, 2025 13:47
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.

In the spec file I'm also missing commandOptionsSchema.parse function calls on tests where command.action is invoked with an options object.

force: !!args.options.force
public getRefinedSchema(schema: typeof options): z.ZodEffects<any> | undefined {
return schema
.refine(options => options.id || options.displayName, {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should also throw an error if both options are used at the same time.

Copy link
Author

Choose a reason for hiding this comment

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

Added mutual exclusion validation using an additional refine constraint that prevents both id and displayName from being specified together. Also added corresponding test case. Changes committed in 72fda67.

Co-authored-by: martinlingstuyl <5267487+martinlingstuyl@users.noreply.github.com>
Copy link
Author

Copilot AI commented Jul 25, 2025

In the spec file I'm also missing commandOptionsSchema.parse function calls on tests where command.action is invoked with an options object.

Added commandOptionsSchema.parse calls to all test cases where command.action is invoked with options object. Changes committed in 72fda67.

@Copilot Copilot AI requested a review from martinlingstuyl July 25, 2025 14:01
Copilot finished work on behalf of martinlingstuyl July 25, 2025 14:01
@martinlingstuyl martinlingstuyl marked this pull request as ready for review August 15, 2025 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate 'entra administrativeunit remove' to Zod
2 participants