Conversation
slabko
force-pushed
the
fix-ci
branch
8 times, most recently
from
c6c6f25 to
7e3e870
Compare
There was a problem hiding this comment.
Pull request overview
Updates CI workflows to run integration tests against a remote ClickHouse Cloud instance by replacing hardcoded credentials/endpoints with GitHub Actions secrets, and adds some extra macOS diagnostics.
Changes:
- Switch Windows (MSVC/Mingw) ClickHouse credentials to
default+ secret password and route the TLS offloader backend to a secret host. - Add/extend
CLICKHOUSE_SECURE_*environment variables for macOS and Linux to use secret host/password. - Add a macOS step to print the OpenSSL default directory.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| .github/workflows/windows_msvc.yml | Use secrets for ClickHouse password and TLS offloader backend host. |
| .github/workflows/windows_mingw.yml | Same as MSVC workflow: use secrets for password and backend host. |
| .github/workflows/macos.yml | Add secure ClickHouse env vars from secrets; print OpenSSL dir; use secret backend host. |
| .github/workflows/linux.yml | Add secure ClickHouse env vars from secrets for TLS integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wget https://github.com/filimonov/go-tlsoffloader/releases/download/v0.1.2/go-tlsoffloader_0.1.2_Darwin_x86_64.tar.gz | ||
| tar -xvzf go-tlsoffloader_0.1.2_Darwin_x86_64.tar.gz | ||
| ./go-tlsoffloader -l localhost:9000 -b github.demo.trial.altinity.cloud:9440 & | ||
| ./go-tlsoffloader -l localhost:9000 -b ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }}:9440 & |
There was a problem hiding this comment.
If INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT is empty (e.g., on fork PRs), this will pass an invalid backend (:9440) and the proxy may exit immediately while the step still succeeds due to running it in the background. Add an explicit check for an empty host (or use a default) so failures are immediate and actionable.
| CLICKHOUSE_SECURE_HOST: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }} | ||
| CLICKHOUSE_SECURE_USER: default | ||
| CLICKHOUSE_SECURE_PASSWORD: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_PASSWORD_SMT }} |
There was a problem hiding this comment.
This workflow runs on pull_request, but these values are sourced from repository secrets. For PRs from forks, secrets.* will be empty, which may cause the TLS integration tests (e.g., ut/ssl_ut.cpp) to connect with empty host/password and fail. Consider providing a fallback to the public demo endpoint/creds or gating remote TLS tests to trusted contexts only.
| CLICKHOUSE_SECURE_HOST: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }} | |
| CLICKHOUSE_SECURE_USER: default | |
| CLICKHOUSE_SECURE_PASSWORD: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_PASSWORD_SMT }} | |
| CLICKHOUSE_SECURE_HOST: >- | |
| ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) | |
| && 'play.clickhouse.com:9440' | |
| || secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }} | |
| CLICKHOUSE_SECURE_USER: default | |
| CLICKHOUSE_SECURE_PASSWORD: >- | |
| ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) | |
| && 'play_password' | |
| || secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_PASSWORD_SMT }} |
| CLICKHOUSE_USER: default | ||
| CLICKHOUSE_PASSWORD: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_PASSWORD_SMT }} |
There was a problem hiding this comment.
These env values come from repository secrets, but this workflow runs on pull_request. For PRs from forks, secrets.* will be empty, which can make the integration tests fail (e.g., auth failures) in a non-obvious way. Consider adding a safe fallback (e.g., demo credentials) or gating the jobs/steps that require secrets to only run when github.event.pull_request.head.repo.full_name == github.repository.
| wget https://github.com/filimonov/go-tlsoffloader/releases/download/v0.1.2/go-tlsoffloader_0.1.2_Windows_x86_64.tar.gz | ||
| tar -xvzf go-tlsoffloader_0.1.2_Windows_x86_64.tar.gz | ||
| ./go-tlsoffloader.exe -l localhost:9000 -b github.demo.trial.altinity.cloud:9440 & | ||
| ./go-tlsoffloader.exe -l localhost:9000 -b ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }}:9440 & |
There was a problem hiding this comment.
If INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT is empty (e.g., on fork PRs where secrets aren’t provided), this command will end up passing -b :9440 and likely fail silently in the background, causing confusing downstream test failures. Consider reading the host from an env var with a default, and/or add a check that fails early when the host is empty.
| CLICKHOUSE_USER: default | ||
| CLICKHOUSE_PASSWORD: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_PASSWORD_SMT }} |
There was a problem hiding this comment.
These env values come from repository secrets, but this workflow runs on pull_request. For PRs from forks, secrets.* will be empty, which can make the integration tests fail (e.g., auth failures) in a non-obvious way. Consider adding a safe fallback (e.g., demo credentials) or gating the jobs/steps that require secrets to only run when github.event.pull_request.head.repo.full_name == github.repository.
| wget https://github.com/filimonov/go-tlsoffloader/releases/download/v0.1.2/go-tlsoffloader_0.1.2_Windows_x86_64.tar.gz | ||
| tar -xvzf go-tlsoffloader_0.1.2_Windows_x86_64.tar.gz | ||
| ./go-tlsoffloader.exe -l localhost:9000 -b github.demo.trial.altinity.cloud:9440 & | ||
| ./go-tlsoffloader.exe -l localhost:9000 -b ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }}:9440 & |
There was a problem hiding this comment.
If INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT is empty (e.g., on fork PRs where secrets aren’t provided), this command will end up passing -b :9440 and likely fail silently in the background, causing confusing downstream test failures. Consider reading the host from an env var with a default, and/or add a check that fails early when the host is empty.
| CLICKHOUSE_USER: default | ||
| CLICKHOUSE_PASSWORD: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_PASSWORD_SMT }} | ||
| CLICKHOUSE_SECURE_HOST: ${{ secrets.INTEGRATIONS_TEAM_TESTS_CLOUD_HOST_SMT }} | ||
| CLICKHOUSE_SECURE_PORT: 9440 | ||
| CLICKHOUSE_SECURE_USER: default |
There was a problem hiding this comment.
This workflow runs on pull_request, but these values are sourced from repository secrets. For PRs from forks, secrets.* will be empty, which can break both the non-TLS and TLS integration test suites. Consider providing defaults (e.g., the existing public demo endpoint/creds) or skipping the remote-integration portions unless the PR comes from the same repository.
No description provided.