Conversation
…ble-role-mt-runner into dev--cygwin-support
Reviewer's Guide by SourceryThis pull request introduces Cygwin support, refactors platform detection, enhances report parsing, and improves the GitHub Actions workflow for testing. It also updates Molecule configurations and dependencies. Sequence diagram for running terminal on different platformssequenceDiagram
participant Ansible
participant Unix
participant Cygwin
participant Windows
alt platform_name == "Unix"
Ansible->>Unix: Runs terminal with mt_runner_mt_terminal_path
else platform_name == "Cygwin"
Ansible->>Cygwin: Runs terminal with mt_runner_mt_terminal_path
else platform_name == "Windows"
Ansible->>Windows: Runs terminal with mt_runner_mt_terminal_path
end
Class diagram for MT5 report parsingclassDiagram
class MT5Report {
<<lookup_plugin>>
+run(terms, variables)
+main(input_file_path, output_file_path, include_titles, type, return_string)
+extract_header_table(html_content)
+extract_orders_table(html_content)
+extract_deals_table(html_content)
+write_to_csv(data, output_file, include_titles, type, return_string)
+write_to_json(html_content, output_file, type, return_string)
+write_opt(input_file, output_file, include_titles, return_string)
+parse_period(value)
+parse_val_prc(value)
+parse_prc_val(value)
+parse_val_of(value)
+parse_val_diff(value)
+parse_time(value)
+convert_value(value)
}
note for MT5Report "This class represents the MT5 report parsing lookup plugin."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @kenorb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on enhancing the project's compatibility with Cygwin and improving the overall development workflow. It includes changes to the .ansible.cfg file to adjust callback settings, updates the .devcontainer/devcontainer.json to add/reorder VS Code extensions and include pre-commit installation, modifies .devcontainer/requirements.txt to add new Python dependencies, introduces new .github files for Copilot instructions and code review guidelines, updates GitHub Actions workflows, adds .gitignore entries, introduces new linting configurations, and updates dependency management files (Pipfile, Pipfile.lock, requirements.yml). Additionally, the parse_mt_report.py lookup plugin is updated to handle different file encodings and improve data extraction, and the main Ansible task file is modified to handle path conversions and terminal execution based on the detected platform.
Highlights
- Cygwin Support: The pull request adds support for Cygwin by adjusting path handling and terminal execution commands based on the detected platform.
- Dependency Management: New Python dependencies like
docker>=7.1,pipenv, andrequests==2.31.0are added to.devcontainer/requirements.txtandPipfileto ensure all necessary tools are available. - Linting and Formatting: New linting configurations (
.yamllint,.markdownlint.yaml) are introduced to enforce code quality and consistency. - GitHub Actions Workflow Improvements: The pull request enhances GitHub Actions workflows by adding screenshot capabilities, improving report generation, and enabling debugging with tmate on failure.
- Code Review and Copilot Instructions: The pull request introduces
.github/prompts/code-review.prompt.mdand.github/copilot-instructions.mdto improve code review quality and provide guidance for Copilot. - Lookup Plugin Enhancements: The
parse_mt_report.pylookup plugin is updated to handle different file encodings (UTF-16LE) and improve data extraction from MT reports.
Changelog
Click here to see the changelog
- .ansible.cfg
- Changed
stdout_callbacktodefaultand addedcallback_result_format = yaml.
- Changed
- .devcontainer/devcontainer.json
- Reordered VS Code extensions and added
github.vscode-github-actions. - Added
pre-commit installtopostCreateCommandto install pre-commit hooks.
- Reordered VS Code extensions and added
- .devcontainer/requirements.txt
- Added
docker>=7.1,pipenv, andrequests==2.31.0as dependencies.
- Added
- .github/copilot-instructions.md
- Added a new file with instructions for Copilot to assist with the project.
- .github/prompts/code-review.prompt.md
- Added a new file with guidelines for code review.
- .github/workflows/check.yml
- Updated
pre-commit/actionversion tov3.0.1.
- Updated
- .github/workflows/molecule.yml
- Added
paths-ignoretopull_requestandpushtriggers to ignore markdown and YAML files. - Added steps to install required tools (x11-apps, imagemagick) and start a background screenshot loop.
- Modified
ANSIBLE_STDOUT_CALLBACKbased onrunner.debugand addedMOLECULE_REPORTenvironment variable. - Added steps to ensure report file permissions are correct and convert the test report to markdown.
- Added conditional debugging with tmate on failure when debug logging is enabled.
- Added
- .gitignore
- Added
*.htmland*.pngto ignore HTML reports and screenshots.
- Added
- .markdownlint.yaml
- Added a new file to configure Markdown linting rules, including line length and allowed HTML elements.
- .yamllint
- Added
.cacheto ignore list. - Added rules for comments, braces, line length, and octal values.
- Added
- Pipfile
- Added
docker = ">=7.1",pipenv = "*",molecule = "*",molecule-docker = "*", andrequests = "==2.31.0"as dependencies.
- Added
- Pipfile.lock
- Updated lock file with new dependencies and versions.
- lookup_plugins/parse_mt_report.py
- Added
argparseimport. - Handled UTF-16LE encoding for OPT reports.
- Added a check for an empty table in OPT reports to avoid errors.
- Added
- meta/main.yml
- Updated the version of
ea31337.metatraderdependency tov1.0.1.
- Updated the version of
- meta/requirements.txt
- Removed file.
- molecule/default/converge.yml
- Replaced
mt_runner_mt_pathwithmt_runner_mt_path_hostinmt_runner_get_url_list.
- Replaced
- molecule/default/molecule.yml
- Enabled
yamllintandansible-lint. - Added
stdout_callbackandresult_formattoconfig_options.defaults. - Added
wine_release: stabletogroup_vars.all.
- Enabled
- molecule/default/requirements.yml
- Removed file.
- molecule/mt4/molecule.yml
- Enabled
yamllintandansible-lint. - Added
stdout_callbackandresult_formattoconfig_options.defaults. - Added
wine_release: stabletogroup_vars.all.
- Enabled
- molecule/mt4/requirements.yml
- Removed file.
- molecule/mt5-ea/molecule.yml
- Enabled
yamllintandansible-lint. - Added
stdout_callbackandresult_formattoconfig_options.defaults. - Added
wine_release: stabletogroup_vars.all.
- Enabled
- molecule/mt5-ea/requirements.yml
- Removed file.
- molecule/requirements.txt
- Replaced specific versions with version ranges and added
pipenv.
- Replaced specific versions with version ranges and added
- requirements.yml
- Added comment about apk module.
- Changed
srctogit+https://github.com/EA31337/ansible-role-metatrader.gitand updated version tov1.0.1.
- tasks/main.yml
- Added debug tasks for
is_unixandplatform_name. - Included
paths.ymlfromea31337.metatraderrole. - Added conditional installation of
winbindpackage based onis_unix. - Added conditional population of MT host paths based on
platform_name. - Added conditional conversion of paths to wine paths based on
is_unix. - Added
/portableargument to terminal execution commands. - Added conditional execution of terminal based on platform (Unix, Cygwin, Windows).
- Added conditional installation of
bs4Python package based onis_unix. - Made
becomeconditional based onis_unixfor copy tasks. - Changed destination of Header report to
.json
- Added debug tasks for
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The name 'Cygwin' is a portmanteau of 'Cygnus' (the original name of Red Hat) and 'Windows'. It provides a Unix-like environment on Windows.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces Cygwin support, enhances report conversion, and updates CI workflows. The changes seem well-structured and address the intended enhancements. However, some areas require closer examination and potential adjustments to ensure robustness and maintainability.
Summary of Findings
- Report Conversion Logic: The report conversion logic in
parse_mt_report.pyinvolves multiple steps of reading, parsing, and writing data. Ensure that error handling is comprehensive and that all edge cases are properly addressed to prevent data loss or corruption. - Conditional Task Execution: The refactoring of task execution based on the platform (Unix, Cygwin, Windows) using conditional statements could lead to increased complexity. Consider using more modular approaches, such as separate task files for each platform, to improve maintainability.
- Screenshot Capture in CI: The screenshot capture loop in CI workflows commits and pushes changes, which might interfere with other CI processes. Evaluate the necessity of this approach and consider alternative methods for storing and accessing screenshots.
Merge Readiness
The pull request introduces significant enhancements and new features. While the changes are generally well-structured, addressing the identified issues related to report conversion, task execution, and CI workflows is crucial before merging. I am unable to approve this pull request, and recommend that it not be merged until the critical and high severity issues are addressed, and that others review and approve this code before merging.
| - name: Clean-up before the test (non-Windows) | ||
| when: ansible_os_family != "Windows" | ||
| - name: Clean-up before the test (Unix) | ||
| when: is_unix and false |
There was a problem hiding this comment.
Why is this block disabled (and false)? If cleanup is needed, it should be enabled, or the block should be removed if it's no longer necessary.
when: is_unix
| if table is None: | ||
| return write_to_csv([["Pass", "Result", "Profit", "Expected Payoff", "Profit Factor", "Recovery Factor", "Sharpe Ratio", "Custom", "Equity DD %"], []], output_file, include_titles, "opt", return_string=return_string) |
There was a problem hiding this comment.
What happens if the file is empty or corrupted? Should there be an explicit check for file validity before attempting to parse it?
| if table is None: | |
| return write_to_csv([["Pass", "Result", "Profit", "Expected Payoff", "Profit Factor", "Recovery Factor", "Sharpe Ratio", "Custom", "Equity DD %"], []], output_file, include_titles, "opt", return_string=return_string) | |
| if not os.path.exists(input_file) or os.path.getsize(input_file) == 0: | |
| return write_to_csv([["Pass", "Result", "Profit", "Expected Payoff", "Profit Factor", "Recovery Factor", "Sharpe Ratio", "Custom", "Equity DD %"], []], output_file, include_titles, "opt", return_string=return_string) | |
| if table is None: |
| loop: "{{ mt_runner_get_url_list }}" | ||
| when: is_unix or platform_name == "Cygwin" |
There was a problem hiding this comment.
Consider adding error handling to the get_url task to handle cases where the URL is unreachable or the file download fails. This could prevent the role from failing silently.
ansible.builtin.get_url:
dest: "{{ item.dest }}"
mode: "{{ item.mode | default('u=rw,g=r') }}"
url: "{{ item.url }}"
rescue:
- name: "Handle get_url failure"
ansible.builtin.debug:
msg: "Failed to download {{ item.url }} to {{ item.dest }}"
| - "{{ mt_runner_mt_terminal_path }}" | ||
| - "{{ '/config:tester.ini' if (mt_runner_mt_version | int) == 5 else 'tester.ini' }}" | ||
| args: | ||
| chdir: '{{ mt_runner_mt_path_host }}' |
There was a problem hiding this comment.
What is the purpose of this task? It seems to be ensuring that the bs4 Python package is installed, but it's being done after the report conversion tasks. It should be done before, to ensure that the parse_mt_report lookup plugin can use it.
| - set_fact: cmd_terminal_async="{{cmd_terminal_async_result}}" | ||
| when: (cmd_terminal_async_result is defined) and (cmd_terminal_async_result.ansible_job_id is defined) | ||
|
|
||
| - name: Check for terminal log | ||
| block: |
There was a problem hiding this comment.
Consider adding a rescue block to handle potential errors during the report conversion process. This could involve logging the error and/or attempting to retry the conversion.
ansible.builtin.copy:
content: |
{{ lookup('parse_mt_report', mt_runner_mt_path ~ '/' ~ mt_runner_bt_mt5_config.Tester.Report, 'orders', True) }}
dest: >-
{{ mt_runner_mt_path }}/{{ mt_runner_bt_mt5_config.Tester.Report }}-Orders.csv
mode: '0644'
rescue:
- name: "Handle copy failure"
ansible.builtin.debug:
msg: "Failed to convert report to CSV (Orders)"
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
We encountered an error and are unable to review this PR. We have been notified and are working to fix it.
You can try again by commenting this pull request with @sourcery-ai review, or contact us for help.
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive Cygwin support to the mt_runner Ansible role, enabling MetaTrader tests to run on Cygwin environments alongside existing Unix and Windows support. The changes introduce platform-specific task execution, enhanced report conversion capabilities, and improved CI workflows for debugging.
Key Changes:
- Platform-conditional task execution for Unix, Cygwin, and Windows environments with appropriate path handling
- New Python lookup plugin for converting MT reports to CSV and JSON formats
- Enhanced CI workflows with debugging capabilities and screenshot capture features
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| tasks/main.yml | Refactored to support three platforms (Unix, Cygwin, Windows) with conditional task execution, path conversion logic, and new report conversion tasks |
| lookup_plugins/parse_mt_report.py | New Python plugin for parsing MT HTML reports and converting them to CSV/JSON formats |
| requirements.yml | Updated metatrader role dependency version and Git URL format |
| molecule/requirements.txt | Updated Docker version requirement and added pipenv and requests dependencies |
| molecule/*/molecule.yml | Standardized Ansible callback configuration across all test scenarios |
| molecule/*/requirements.yml | Removed redundant role dependencies (now centralized) |
| molecule/default/converge.yml | Updated path variable to use host-specific path variant |
| Pipfile & Pipfile.lock | Added for Python dependency management with specific version pins |
| .github/workflows/molecule.yml | Enhanced with debug mode, screenshot capture loop, and report conversion |
| .github/workflows/check.yml | Updated pre-commit action version |
| .github/prompts/code-review.prompt.md | New code review guidelines for the Ansible role |
| .github/copilot-instructions.md | New Copilot configuration with project-specific guidelines |
| .devcontainer/* | Updated requirements and configuration for development environment |
| .markdownlint.yaml | New markdown linting configuration |
| .gitignore | Added HTML files to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ToDate: 2024.01.11 | ||
| mt_runner_get_url_list: | ||
| - dest: "{{ mt_runner_mt_path }}/MQL5/Experts" | ||
| - dest: "{{ mt_runner_mt_path_host }}/MQL5/Experts" |
There was a problem hiding this comment.
The path formatting is inconsistent. Line 28 uses 'mt_runner_mt_path_host' while line 29 in the original code uses 'mt_runner_mt_path'. For Cygwin and Windows platforms, these should both use the host path variant to ensure consistency. Consider whether this variable name change is intentional and correctly reflects the platform requirements.
| - set_fact: cmd_terminal_async="{{cmd_terminal_async_result}}" | ||
| when: (cmd_terminal_async_result is defined) and (cmd_terminal_async_result.ansible_job_id is defined) |
There was a problem hiding this comment.
Missing module name before 'set_fact'. Ansible task declarations must include the module name. This should be 'ansible.builtin.set_fact:' instead of just 'set_fact:'.
| - set_fact: cmd_terminal_async="{{cmd_terminal_async_result}}" | ||
| when: (cmd_terminal_async_result is defined) and (cmd_terminal_async_result.ansible_job_id is defined) |
There was a problem hiding this comment.
Missing module name before 'set_fact'. Ansible task declarations must include the module name. This should be 'ansible.builtin.set_fact:' instead of just 'set_fact:'.
| - name: Clean-up before the test (Unix) | ||
| when: is_unix and false |
There was a problem hiding this comment.
The Clean-up block has been disabled with 'when: is_unix and false', which means it will never execute. This appears to be debug code that should either be removed or have a proper condition. Dead code should not be committed to the codebase.
| become: "{{ is_unix | bool }}" | ||
| become_user: root | ||
|
|
||
| - name: Converts report to CSV (Header) |
There was a problem hiding this comment.
The task name is misleading. The comment says "Converts report to CSV (Header)" but the output file is saved as JSON format (.json extension) and the function write_to_json is being used. The task name should be "Converts report to JSON (Header)" to accurately reflect what it does.
| - name: Converts report to CSV (Header) | |
| - name: Converts report to JSON (Header) |
| - set_fact: cmd_terminal_async="{{cmd_terminal_async_result}}" | ||
| when: (cmd_terminal_async_result is defined) and (cmd_terminal_async_result.ansible_job_id is defined) |
There was a problem hiding this comment.
Missing module name before 'set_fact'. Ansible task declarations must include the module name. This should be 'ansible.builtin.set_fact:' instead of just 'set_fact:'.
| ansible.windows.win_command: | ||
| argv: | ||
| - "{{ mt_runner_cmd_prefix }}" | ||
| - "{{ mt_runner_mt_terminal_path }}" |
There was a problem hiding this comment.
The Windows terminal execution is missing the '/portable' flag that is present in both Unix and Cygwin versions. This inconsistency could lead to different behavior across platforms. If this is intentional, it should be documented; otherwise, the flag should be added for consistency.
| - "{{ mt_runner_mt_terminal_path }}" | |
| - "{{ mt_runner_mt_terminal_path }}" | |
| - '/portable' |
| - name: Start background screenshot loop | ||
| run: | | ||
| cd $GITHUB_WORKSPACE | ||
| mkdir -p screenshots | ||
| echo "Starting screenshot loop..." | ||
| while true; do | ||
| sleep 120 | ||
| TIMESTAMP=$(date +"%Y%m%d_%H%M%S") | ||
| SCREENSHOT_FILE="screenshots/latest_screenshot.png" | ||
|
|
||
| # Take screenshot | ||
| DISPLAY=:0 xwd -root -out temp_screenshot.xwd || true | ||
| convert temp_screenshot.xwd "$SCREENSHOT_FILE" || true | ||
| rm temp_screenshot.xwd || true | ||
|
|
||
| if [ -f "$SCREENSHOT_FILE" ]; then | ||
| # Commit latest screenshot with amend | ||
| git add "$SCREENSHOT_FILE" | ||
| git commit --amend -m "Update latest screenshot taken at $TIMESTAMP" | ||
| git push --force-with-lease | ||
| fi | ||
| done & |
There was a problem hiding this comment.
The background screenshot loop has several issues: 1) It uses git operations (add, commit, push) within CI that could fail or cause conflicts. 2) Using --force-with-lease can overwrite other changes. 3) The loop runs indefinitely without proper cleanup. 4) This approach is unconventional for CI debugging. Consider using CI artifacts instead of git commits for screenshots.
| - mt4 | ||
| - mt5-ea | ||
| steps: | ||
| - name: Debug with tmate |
There was a problem hiding this comment.
The tmate debug action is configured to run at the start of every job with 'detached: true', which means debugging will always be enabled. This should be conditional (e.g., only when runner.debug == '1' or on failure) to avoid leaving debug sessions open unnecessarily and potentially exposing the CI environment.
| - name: Debug with tmate | |
| - name: Debug with tmate | |
| if: ${{ runner.debug == '1' }} |
| return write_to_csv([columns, data], output_file, include_titles, "opt", return_string=return_string) | ||
|
|
||
| def main(input_file_path, output_file_path, include_titles = False, type = None, return_string=False): | ||
| if type == None: |
There was a problem hiding this comment.
Testing for None should use the 'is' operator.
| if type == None: | |
| if type is None: |
2 participants
Summary by Sourcery
Adds support for Cygwin to the mt_runner role, allowing the execution of MetaTrader tests on Cygwin environments. This includes platform-specific configurations, path conversions, and command execution. Additionally, the pull request enhances report conversion capabilities and updates CI workflows for improved testing and reporting.
Enhancements:
Tests: