Skip to content
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

Cleanup docker compose #3854

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Cleanup docker compose #3854

merged 2 commits into from
Aug 21, 2024

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Aug 21, 2024

What does this PR do?

  1. Fix TEST_PRESTO_HOST from .env to use 127.0.0.1
  2. Remove obsolete common-environment-3x anchor (They are identical to common-environment, except presto host which is fixed from .env)
  3. Extract common-depends-on anchor
  4. Remove obsolete version field from docker-compose.yml

Why

I stumble on this overwriting complexity (with layers of overwriting environment) when testing with telemetry.

@TonyCTHsu TonyCTHsu added the dev/internal Other internal work that does not need to be included in the changelog label Aug 21, 2024
@TonyCTHsu TonyCTHsu requested a review from a team as a code owner August 21, 2024 11:24
@@ -28,7 +27,6 @@ services:
TEST_OPENSEARCH_PORT: 9200
TEST_POSTGRES_HOST: postgres
TEST_PRESTO_HOST: presto
TEST_PRESTO_PORT: 8080
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this line be removed? The env var is still referenced here.

Copy link
Contributor Author

@TonyCTHsu TonyCTHsu Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed because it is provided by .env file.

For each service, we are applying environment variables by providing env_file and overwrite with environment.

For example:

    env_file: ./.env
    environment: 
      -  ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! I missed the .env file

@pr-commenter
Copy link

pr-commenter bot commented Aug 21, 2024

Benchmarks

Benchmark execution time: 2024-08-21 11:58:15

Comparing candidate commit 7622101 in PR branch tonycthsu/refactor-dev-env with baseline commit b7b99b2 in branch master.

Found 0 performance improvements and 3 performance regressions! Performance is the same for 20 metrics, 2 unstable metrics.

scenario:profiler - estimated profiler gc per minute (sample 60000 times + serialize result)

  • 🟥 throughput [-0.106op/s; -0.096op/s] or [-2.570%; -2.317%]

scenario:profiler - profiler gc

  • 🟥 throughput [-11985.722op/s; -11413.940op/s] or [-3.239%; -3.085%]

scenario:profiler - sample timeline=false

  • 🟥 throughput [-0.262op/s; -0.229op/s] or [-3.612%; -3.164%]

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.86%. Comparing base (d2498fb) to head (7622101).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3854      +/-   ##
==========================================
+ Coverage   97.83%   97.86%   +0.02%     
==========================================
  Files        1264     1269       +5     
  Lines       75725    75868     +143     
  Branches     3729     3736       +7     
==========================================
+ Hits        74084    74245     +161     
+ Misses       1641     1623      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TonyCTHsu TonyCTHsu merged commit 84e92fa into master Aug 21, 2024
186 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/refactor-dev-env branch August 21, 2024 12:54
@github-actions github-actions bot added this to the 2.3.0 milestone Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/internal Other internal work that does not need to be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants