Promote Elasticsearch to a native GitHub Actions service#1008
Promote Elasticsearch to a native GitHub Actions service#1008mattmenefee wants to merge 1 commit intotoptal:masterfrom
Conversation
mattmenefee
force-pushed
the
ci-test-infrastructure-improvements
branch
3 times, most recently
from
dc8bcb5 to
f4afb4c
Compare
mattmenefee
force-pushed
the
ci-test-infrastructure-improvements
branch
2 times, most recently
from
688fe0d to
48236a8
Compare
bbatsov
left a comment
bbatsov
left a comment
There was a problem hiding this comment.
This bundles a lot of unrelated changes into one PR, which makes it hard to review. I'd suggest trimming it to the core CI improvements (the services: block for ES, concurrency, timeouts) and the drop_indices fix, then sending the rest as separate PRs. One logical change per commit, please.
A few specific things:
- The canary specs (
elasticsearch_compatibility_spec.rb) test third-party library internals, not Chewy. If the ES gem breaks its own API, that's their problem. I'd drop these entirely. - The README Ruby version change is unrelated to CI work -- separate PR.
- Changelog should use
## master (unreleased), not## 8.0.2 (unreleased). Version numbers are decided at release time.
| # Security disabled for local testing only; enable in production | ||
| - xpack.security.enabled=false | ||
| - action.destructive_requires_name=false | ||
| ports: |
There was a problem hiding this comment.
Is this actually needed? drop_indices passes an explicit list of index names, not a wildcard.
There was a problem hiding this comment.
Good catch! You're right, drop_indices passes explicit names so this setting has no effect. I'll remove it.
| discovery.type: single-node | ||
| xpack.security.enabled: 'false' | ||
| ES_JAVA_OPTS: '-Xms500m -Xmx500m' | ||
| # Set health checks to wait until Elasticsearch has started |
There was a problem hiding this comment.
green is stricter than needed for a single-node test cluster. yellow is more conventional for CI and avoids potential flakiness.
There was a problem hiding this comment.
Fair point. I originally used green for consistency with the wait_for_status: 'green' setting in spec_helper.rb, but you're right that they serve different purposes. The CI health check just needs to know the cluster is ready to accept requests, so yellow is the better threshold here. I'll change it.
mattmenefee
force-pushed
the
ci-test-infrastructure-improvements
branch
from
24c5bf8 to
98bc129
Compare
The previous CI setup launched ES via `docker compose up` with a `sleep 15` wait, which was fragile and slow. Promote ES to a proper GitHub Actions `services:` block with a built-in health check (`wait_for_status=yellow`) for reliable startup detection. - Add `concurrency` group to cancel in-progress runs on PR updates - Add `timeout-minutes` to both jobs (15 test, 10 rubocop) - Add `permissions: contents: read` for least-privilege - Rename test job from `ruby-3` to `test` (Ruby 4.0 is in the matrix) - Switch docker-compose image to `docker.elastic.co` registry and parameterize the version via `ES_VERSION` env var
mattmenefee
force-pushed
the
ci-test-infrastructure-improvements
branch
from
98bc129 to
4b0aa9e
Compare
|
@bbatsov Thanks for the review! I've addressed your feedback:
|
mattmenefee
changed the title
2 participants
Summary
services:block with a built-in health check (wait_for_status=yellow), replacing the fragiledocker compose+sleep 15approachdocker.elastic.coregistry and parameterize the version viaES_VERSIONenv var indocker-compose.ymlruby-3totest(Ruby 4.0 is now in the matrix)timeout-minutesto both jobs (15 for tests, 10 for rubocop)concurrencygroup to cancel in-progress CI runs on PR updatesTest plan
docker.elastic.co