chore: migrate diagnose & capture-screen to multi-gateway TARGETS_JSON#88
Conversation
Replace hardcoded single-gateway org vars with repo-level IB_GATEWAY_TARGETS_JSON multi-gateway resolution. - diagnose.yml: add target input, resolve from TARGETS_JSON - capture-screen.yml: add target input, resolve from TARGETS_JSON - Both now support per-gateway GCP project/auth/instance/zone Co-Authored-By: Claude <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
🤖 Codex PR ReviewPlease ensure a human reviewer checks this PR before merging. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9a075f356
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with open(os.environ["GITHUB_OUTPUT"], "a") as f: | ||
| f.write("matrix=" + json.dumps({"target": selected}, separators=(",", ":")) + "\n") | ||
| print("Targets: " + ", ".join(t["name"] for t in selected)) | ||
| shell: python3 |
There was a problem hiding this comment.
Use a valid Python shell for target resolution
Both migrated resolve jobs set shell: python3, but GitHub Actions only treats python as the built-in Python shell and custom shell strings must include {0} (per the workflow syntax docs). On hosted runners this is rejected as an invalid shell option before the Python code runs, so the workflow never produces needs.resolve.outputs.matrix; use shell: python or shell: python3 {0} here and in the matching diagnose.yml resolve step.
Useful? React with 👍 / 👎.
| strategy: | ||
| matrix: ${{ fromJson(needs.resolve.outputs.matrix) }} |
There was a problem hiding this comment.
Give each capture matrix artifact a unique name
When target is left as all and the matrix contains more than one gateway, each matrix job still reaches the upload step with the same artifact name ibkr-gateway-screen. The upload-artifact action rejects duplicate artifact names from matrix/multiple jobs, so only the first target can upload and the rest fail instead of producing screenshots; include matrix.target.name in the artifact name.
Useful? React with 👍 / 👎.
| TARGET_NAME: ${{ matrix.target.name }} | ||
| GCP_PROJECT_ID: ${{ matrix.target.gcp_project_id }} | ||
| GCP_WORKLOAD_IDENTITY_PROVIDER: ${{ matrix.target.gcp_workload_identity_provider }} | ||
| GCP_WORKLOAD_IDENTITY_SERVICE_ACCOUNT: ${{ matrix.target.gcp_workload_identity_service_account }} | ||
| GCE_USER: ${{ matrix.target.gce_user }} | ||
| GCE_INSTANCE_NAME: ${{ matrix.target.gce_instance_name }} | ||
| GCE_ZONE: ${{ matrix.target.gce_zone }} | ||
| SSH_PRIVATE_KEY_SECRET_NAME: ${{ matrix.target.ssh_private_key_secret_name }} |
There was a problem hiding this comment.
Pass the selected container name to capture
This matrix env block only carries VM and SSH fields from TARGETS_JSON, while the capture commands later still run against the literal container ib-gateway. Targets deployed by main.yml can set container_name, so selecting any non-default gateway captures the wrong/default container or fails even though the VM target was resolved correctly; pass matrix.target.container_name through and default it to ib-gateway in the remote command.
Useful? React with 👍 / 👎.
| TARGET_NAME: ${{ matrix.target.name }} | ||
| GCP_PROJECT_ID: ${{ matrix.target.gcp_project_id }} | ||
| GCP_WORKLOAD_IDENTITY_PROVIDER: ${{ matrix.target.gcp_workload_identity_provider }} | ||
| GCP_WORKLOAD_IDENTITY_SERVICE_ACCOUNT: ${{ matrix.target.gcp_workload_identity_service_account }} | ||
| GCE_USER: ${{ matrix.target.gce_user }} | ||
| GCE_INSTANCE_NAME: ${{ matrix.target.gce_instance_name }} | ||
| GCE_ZONE: ${{ matrix.target.gce_zone }} | ||
| DEPLOY_PATH: ${{ matrix.target.deploy_path }} | ||
| SSH_PRIVATE_KEY_SECRET_NAME: ${{ matrix.target.ssh_private_key_secret_name }} |
There was a problem hiding this comment.
Pass target container metadata to diagnostics
The diagnostic job now selects a target, but this env block drops the target's container_name and unit_suffix; the remote script below still checks hard-coded resources such as docker exec ib-gateway and ibkr-2fa-bot.*. For targets deployed with non-default container/unit names, diagnostics will inspect the default gateway or report missing services instead of the selected gateway, so carry these target fields through as main.yml does.
Useful? React with 👍 / 👎.
| if isinstance(loaded, dict): | ||
| for name, config in loaded.items(): | ||
| cfg = dict(config) | ||
| cfg["name"] = name |
There was a problem hiding this comment.
Preserve names embedded in object targets
The resolver now overwrites every object-form entry's name with the map key, while .github/workflows/main.yml uses target.setdefault("name", name) for the same IB_GATEWAY_TARGETS_JSON schema. If the repo variable already relies on an explicit name inside the target object (which main accepts), manual target selection by that gateway name fails here and the matrix runs under the wrong name; use setdefault and validate names consistently.
Useful? React with 👍 / 👎.
| DEPLOY_PATH: ${{ matrix.target.deploy_path }} | ||
| SSH_PRIVATE_KEY_SECRET_NAME: ${{ matrix.target.ssh_private_key_secret_name }} |
There was a problem hiding this comment.
Carry target port settings into diagnostics
The migrated diagnostic env only passes deploy/SSH fields, but multi-gateway targets in main.yml can publish non-default live_host_port, paper_host_port, and vnc_host_port; the remote diagnostics below still greps only 4001|4002|4003|4004|5900. For any selected target using custom host ports, the report falsely shows the gateway ports as absent even though the gateway is listening on its configured ports, so propagate the port fields and build the port checks from them.
Useful? React with 👍 / 👎.
改动
diagnose.yml和capture-screen.yml从硬编码单网关 org vars 迁移到 repo 级IB_GATEWAY_TARGETS_JSON多网关架构。变更
target输入参数,支持选择或全选 gatewaymain.yml的多网关模式保持一致迁移后
IB_GATEWAY_TARGETS_JSON读配置IB_GATEWAY_*单独 var 可在验证后删除IB_GATEWAY_*var 仍被 IBKR Platform 引用,暂不动🤖 Generated with Claude Code