-
Notifications
You must be signed in to change notification settings - Fork 1
Mfal/copilot files #440
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
base: main
Are you sure you want to change the base?
Mfal/copilot files #440
Conversation
- Add -O for --oid (organization ID) - Add -W for --wid (workspace ID) - Add -S for --sid (solution ID) - Add -d for --did (dataset ID) - Add -r for --rid (runner ID) - Add -R for --rnid (run ID) - Add comprehensive tests for API short-form options
- Add -N for --namespace in apply and destroy macros - Append Macro test class skeleton - Update Step 2 checklist and verification
…form -w for --workspace-id in multiple PowerBI command files (without testing powerbi commands)
- Added short-form alternatives (-X) for all applicable CLI options across Babylon commands to enhance usability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds short-form CLI option support to the Babylon CLI tool, along with comprehensive documentation, tests, and GitHub Copilot configuration files for maintaining code quality and architectural consistency.
Changes:
- Adds short-form options (e.g.,
-O,-w,-e,-l) to API, PowerBI, Azure, and main CLI commands - Introduces comprehensive test coverage for short-form options via two test files
- Adds GitHub Copilot instructions and prompt templates for structured development workflows
- Documents architectural patterns, code exemplars, and implementation details
Reviewed changes
Copilot reviewed 41 out of 43 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/test_option_shortform.py |
Comprehensive tests verifying short-form options appear in help text for API, PowerBI, Azure, and main CLI commands |
tests/unit/test_help_shortform.py |
Test ensuring -h and --help produce identical output across all commands |
plans/short-form-options/implementation.md |
Detailed step-by-step implementation guide with verification checklists |
plans/short-form-options/changes.md |
Summary of all short-form option changes and conflicts |
plans/short-form-options/exemplars.md |
Code quality guidelines and architectural patterns |
plans/doc_copilot/plan.md |
Development planning workflow documentation |
plans/doc_copilot/Project_Architecture_Blueprint.md |
Comprehensive architecture documentation |
Babylon/main.py |
Adds -h help shortform and -l for --log-path |
Babylon/commands/powerbi/**/*.py |
Adds -w for --workspace-id and -e for --email across PowerBI commands |
Babylon/commands/azure/token/*.py |
Adds -e for --email in Azure token commands |
Babylon/commands/api/*.py |
Adds short forms for organization, workspace, solution, dataset, runner, and run ID options |
Babylon/commands/macro/apply.py |
Adds -N for --namespace option |
Babylon/commands/macro/destroy.py |
Adds -N for --namespace option |
.github/prompts/*.md |
GitHub Copilot prompt templates for planning, implementation, and blueprint generation |
.github/copilot-instructions.md |
Project-specific coding instructions for GitHub Copilot |
babylon_info.log, babylon_error.log |
Empty log files (should not be committed) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/copilot-instructions.md
Outdated
| - [Project_Architecture_Blueprint.md](Project_Architecture_Blueprint.md): For a deep dive into the system's architecture, layers, and design decisions. | ||
| - [exemplars.md](exemplars.md): For high-quality code examples demonstrating standard patterns and best practices. |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file paths referenced in lines 56-57 are incorrect. The files are located at plans/doc_copilot/Project_Architecture_Blueprint.md and plans/short-form-options/exemplars.md respectively, not in the root directory. These paths should be updated to reflect the actual file locations so that developers can find the referenced documentation.
| - [Project_Architecture_Blueprint.md](Project_Architecture_Blueprint.md): For a deep dive into the system's architecture, layers, and design decisions. | |
| - [exemplars.md](exemplars.md): For high-quality code examples demonstrating standard patterns and best practices. | |
| - [Project_Architecture_Blueprint.md](plans/doc_copilot/Project_Architecture_Blueprint.md): For a deep dive into the system's architecture, layers, and design decisions. | |
| - [exemplars.md](plans/short-form-options/exemplars.md): For high-quality code examples demonstrating standard patterns and best practices. |
tests/unit/test_option_shortform.py
Outdated
| result = runner.invoke(main, command_path + ["--help"]) | ||
| assert result.exit_code == 0 | ||
| assert "-d" in result.output, f"-d not found in {' '.join(command_path)} help" | ||
| assert "--did" in result.output, f"--did not found in {' '.join(command_path)} help" |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message contains a grammatical error. It should read "not found" instead of "not found" (missing the word "not").
Babylon/commands/macro/apply.py
Outdated
| # If a namespace is provided, set it for the environment | ||
| if namespace: | ||
| env.get_ns_from_text(content=namespace) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespace parameter logic has a potential issue. When a namespace is provided via the -N/--namespace flag, it sets the environment namespace globally at line 73. However, the deploy_objects function also calls env.get_ns_from_text(content=namespace) for each resource (see line 23 in deploy_organization.py, deploy_solution.py, and deploy_workspace.py), where the namespace comes from the YAML file. This means that:
- If a user provides
-N namespace1but the YAML files contain different namespace values, the namespace from the YAML file will override the CLI-provided namespace for each resource. - The CLI-provided namespace is set once at the beginning but can be overwritten multiple times by YAML files.
This could lead to unexpected behavior. Consider either:
- Making the CLI namespace take precedence over YAML namespace values, or
- Documenting clearly that YAML namespace values override the CLI namespace, or
- Validating that CLI and YAML namespaces match if both are provided
Babylon/commands/macro/destroy.py
Outdated
| @retrieve_state | ||
| @option("-N", "--namespace", "namespace", required=False, type=str, help="The namespace to destroy") | ||
| @option("--include", "include", multiple=True, type=str, help="Specify the resources to destroy.") | ||
| @option("--exclude", "exclude", multiple=True, type=str, help="Specify the resources to exclude from destroction.") |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error in the help text: "destroction" should be "destruction".
| @option("--exclude", "exclude", multiple=True, type=str, help="Specify the resources to exclude from destroction.") | |
| @option("--exclude", "exclude", multiple=True, type=str, help="Specify the resources to exclude from destruction.") |
Babylon/commands/api/workspace.py
Outdated
| @pass_keycloak_token() | ||
| @option("--oid", "organization_id", required=True, type=str, help="Organization ID") | ||
| @option("-O", "--oid", "organization_id", required=True, type=str, help="Organization ID") | ||
| @option("--sid", "solution_id", required=True, type=str, help="Solution ID") |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --sid option is missing its short form -S. According to the documentation in the PR (changes.md and implementation.md), the --sid option should have the short form -S added. This is inconsistent with other files where short forms were added (e.g., line 63 in solution.py has -S). The option on line 35 should be @option("-S", "--sid", ...) to match the pattern used elsewhere.
| @option("--sid", "solution_id", required=True, type=str, help="Solution ID") | |
| @option("-S", "--sid", "solution_id", required=True, type=str, help="Solution ID") |
Babylon/commands/macro/destroy.py
Outdated
| @retrieve_state | ||
| @option("-N", "--namespace", "namespace", required=False, type=str, help="The namespace to destroy") | ||
| @option("--include", "include", multiple=True, type=str, help="Specify the resources to destroy.") | ||
| @option("--exclude", "exclude", multiple=True, type=str, help="Specify the resources to exclude from destroction.") | ||
| def destroy( | ||
| state: dict, | ||
| namespace: str | None, | ||
| include: tuple[str], | ||
| exclude: tuple[str], | ||
| ): | ||
| """Macro Destroy""" | ||
| # If a namespace is provided, set it for the environment before using state | ||
| if namespace: | ||
| env.get_ns_from_text(content=namespace) |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @retrieve_state decorator is called before the namespace is set. The decorator at line 42 will retrieve the state based on the current environment namespace, but the namespace parameter is only processed at lines 54-55 after the state has already been retrieved. This means the state will be retrieved for the wrong namespace.
The namespace parameter should be processed before @retrieve_state is executed. Consider either:
- Moving the namespace logic into the decorator chain
- Removing
@retrieve_stateand manually retrieving the state after setting the namespace - Creating a custom decorator that handles both namespace setting and state retrieval in the correct order
Babylon/commands/api/runner.py
Outdated
| @pass_keycloak_token() | ||
| @option("--oid", "organization_id", required=True, type=str, help="Organization ID") | ||
| @option("-O", "--oid", "organization_id", required=True, type=str, help="Organization ID") | ||
| @option("--sid", "solution_id", required=True, type=str, help="Solution ID") |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --sid option is missing its short form -S. According to the documentation in the PR (changes.md and implementation.md), the --sid option should have the short form -S added. This should be @option("-S", "--sid", ...) to be consistent with the changes made in solution.py and other files.
| @option("--sid", "solution_id", required=True, type=str, help="Solution ID") | |
| @option("-S", "--sid", "solution_id", required=True, type=str, help="Solution ID") |
Babylon/commands/api/dataset.py
Outdated
| @option("-O", "--oid", "organization_id", required=True, type=str, help="Organization ID") | ||
| @option("-W", "--wid", "workspace_id", required=True, type=str, help="Workspace ID") | ||
| @option("-d", "--did", "dataset_id", required=True, type=str, help="Dataset ID") | ||
| @option("--dpid", "dataset_part_id", required=True, type=str, help="Dataset Part ID") |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --dpid option is missing its short form -p. This should be @option("-p", "--dpid", ...) to be consistent with the documented changes.
Babylon/commands/api/dataset.py
Outdated
| @option("-O", "--oid", "organization_id", required=True, type=str, help="Organization ID") | ||
| @option("-W", "--wid", "workspace_id", required=True, type=str, help="Workspace ID") | ||
| @option("-d", "--did", "dataset_id", required=True, type=str, help="Dataset ID") | ||
| @option("--dpid", "dataset_part_id", required=True, type=str, help="Dataset Part ID") |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --dpid option is missing its short form -p. This should be @option("-p", "--dpid", ...) to be consistent with the documented changes.
| @option("--dpid", "dataset_part_id", required=True, type=str, help="Dataset Part ID") | |
| @option("-p", "--dpid", "dataset_part_id", required=True, type=str, help="Dataset Part ID") |
tests/unit/test_help_shortform.py
Outdated
| @@ -0,0 +1,42 @@ | |||
| import pytest | |||
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'pytest' is not used.
- Added short-form options for organization ID (-O), workspace ID (-W), solution ID (-S), dataset ID (-d), dataset part ID (-p), runner ID (-r), and run ID (-R) across various API commands. - Updated macro commands to remove unused namespace option and cleaned up related code. - Enhanced tests to verify the presence of short-form options in help outputs for API, PowerBI, and Azure commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 37 out of 40 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - [x] `-l` and `--log-path` both appear in help output (verified via CliRunner) | ||
| - [x] No syntax errors in modified files | ||
|
|
||
| ### Step 6 STOP & COMMITsource |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text "STOP & COMMITsource" appears to have a typo. It should be "STOP & COMMIT" (the word "source" seems to be accidentally appended).
| 1. **Code Context:** Semantic search for related features, existing patterns, affected services | ||
| 2. **Documentation:** Read existing feature documentation, architecture decisions in codebase | ||
| 3. **Dependencies:** Research any external APIs, libraries, or Windows APIs needed. Use #context7 if available to read relevant documentation. ALWAYS READ THE DOCUMENTATION FIRST. | ||
| 4. **Patterns:** Identify how similar features are implemented in ResizeMe |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prompt references "ResizeMe" on line 78, which appears to be from a different project. This should be updated to reference "Babylon" to match the current project context.
| 4. **Patterns:** Identify how similar features are implemented in ResizeMe | |
| 4. **Patterns:** Identify how similar features are implemented in Babylon |
| 4. Ask clarifying questions for any `[NEEDS CLARIFICATION]` sections | ||
| 5. MANDATORY: Pause for feedback | ||
| 6. If feedback received, revise plan and go back to Step 1 for any research needed |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The step numbering in the Plan Generation section is incorrect. It goes from step 2 to step 4, skipping step 3. The list should be numbered 1, 2, 3, 4, 5, 6.
| 4. Ask clarifying questions for any `[NEEDS CLARIFICATION]` sections | |
| 5. MANDATORY: Pause for feedback | |
| 6. If feedback received, revise plan and go back to Step 1 for any research needed | |
| 3. Ask clarifying questions for any `[NEEDS CLARIFICATION]` sections | |
| 4. MANDATORY: Pause for feedback | |
| 5. If feedback received, revise plan | |
| 6. Go back to Step 1 for any research needed |
| All short-form options are covered by tests in: | ||
| - `tests/unit/test_option_shortform.py` | ||
| - `tests/unit/test_help_shortform.py` | ||
|
|
||
| Run all tests: | ||
| ```bash | ||
| pytest tests/unit/test_option_shortform.py tests/unit/test_help_shortform.py -v | ||
| ``` |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation.md document references test files tests/unit/test_option_shortform.py and tests/unit/test_help_shortform.py throughout (lines 261-297, 337-350, 401-422, 473-489, and in the final verification sections), but these test files do not exist in the repository.
Either:
- Create the referenced test files as part of this PR
- Remove references to these non-existent test files from the documentation
- Update the documentation to clarify that tests are planned but not yet implemented
| ### Macro Commands (Steps 1-2, Previously Completed) | ||
|
|
||
| | Long Option | Short Option | Files Modified | | ||
| |-------------|--------------|----------------| | ||
| | `--namespace` | `-N` | apply.py, deploy.py, destroy.py | | ||
|
|
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states that Macro commands have a --namespace option with -N short form added in steps 1-2, but this is inaccurate. The macro commands (apply, destroy, init) don't expose a --namespace CLI option. The namespace parameter exists only in internal functions like deploy_organization(), deploy_solution(), and deploy_workspace(), which are not directly accessible as CLI commands.
Update the documentation to accurately reflect which commands received short-form options, or clarify that these are internal function parameters rather than CLI options.
| ## Total Summary | ||
|
|
||
| - **Total options with short forms:** 11 unique options | ||
| - **Total files modified:** ~35 files | ||
| - **Commands affected:** API, Macro, PowerBI, Azure, Main CLI |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The total count states "11 unique options" with commands affected including "Macro", but based on the codebase analysis, macro commands don't have the documented namespace short-form option. The count should be "10 unique options" and macro commands should be removed from the affected commands list, as already corrected in the changes.md file (line 43).
No description provided.