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

Improved env var interpolation #164

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

YassinEldeeb
Copy link
Member

@YassinEldeeb YassinEldeeb commented Nov 27, 2023

Interpolation Progress

  • Eliminate the need for a EnvVarsFetcher abstraction, in favor of closures as a second argument to load_config.
  • Correct the regex pattern to function properly
  • Implement escaping of dollar signs using backslashes for literal dollar sign interpretation in configuration files.
  • Support default values in environment variable interpolation syntax to provide fallbacks when variables are not set.
  • Prioritize explicit environment variable values over provided default values.
  • Full unit test coverage to verify proper handling of all common scenarios.
  • Update documentation.
  • Removed lazy_static dependency, since we don't really need it, we copy over the variables anyways.

Better Interpolation Logging

  • Initialize the logger subscriber before config file parsing with info as the default level, then modify the log-level afterwards
  • Flip all println!(...) before logger subscriber initialization including the config parser's code into using info!, error! and warning!

Copy link

💻 Website Preview

The latest changes are available as preview in: https://d2154a0a.conductor-t2.pages.dev

Copy link

github-actions bot commented Nov 27, 2023

✅ Benchmark Results

     data_received..................: 210 MB  2.6 MB/s
     data_sent......................: 652 MB  8.1 MB/s
     http_req_blocked...............: avg=2.81µs  min=611ns   med=1.45µs  max=34.87ms p(90)=1.94µs  p(95)=2.17µs 
     http_req_connecting............: avg=611ns   min=0s      med=0s      max=31.78ms p(90)=0s      p(95)=0s     
   ✓ http_req_duration..............: avg=4.31ms  min=59.5µs  med=2.32ms  max=76.69ms p(90)=11.19ms p(95)=15.54ms
       { expected_response:true }...: avg=4.31ms  min=59.5µs  med=2.32ms  max=76.69ms p(90)=11.19ms p(95)=15.54ms
   ✓ http_req_failed................: 0.00%   ✓ 0            ✗ 1654408
     http_req_receiving.............: avg=41.24µs min=6.48µs  med=15.92µs max=49.44ms p(90)=22.82µs p(95)=76.02µs
     http_req_sending...............: avg=15.78µs min=4.02µs  med=7.94µs  max=49.7ms  p(90)=10.8µs  p(95)=14.84µs
     http_req_tls_handshaking.......: avg=0s      min=0s      med=0s      max=0s      p(90)=0s      p(95)=0s     
     http_req_waiting...............: avg=4.25ms  min=41.32µs med=2.28ms  max=68.18ms p(90)=11.09ms p(95)=15.42ms
     http_reqs......................: 1654408 20680.020186/s
     iteration_duration.............: avg=4.41ms  min=99.76µs med=2.4ms   max=85.72ms p(90)=11.33ms p(95)=15.71ms
     iterations.....................: 1654408 20680.020186/s
     vus............................: 1       min=1          max=199  
     vus_max........................: 200     min=200        max=200  

@YassinEldeeb YassinEldeeb changed the base branch from master to interpolate-vars November 27, 2023 01:52
Copy link

github-actions bot commented Nov 27, 2023

🐋 This PR was built and pushed to the following Docker images:

Docker Bake metadata
{
"conductor": {
  "containerimage.config.digest": "sha256:b5ded1596a2fd672635efe6ee31670a5feff7e6cbcb89288ef66d507f977fbcb",
  "containerimage.descriptor": {
    "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
    "digest": "sha256:1deae3968a8767dbc53d26007016f0f713ab612e5c60ba69c3e8f4e8417632dc",
    "size": 901,
    "platform": {
      "architecture": "amd64",
      "os": "linux"
    }
  },
  "containerimage.digest": "sha256:1deae3968a8767dbc53d26007016f0f713ab612e5c60ba69c3e8f4e8417632dc",
  "image.name": "ghcr.io/the-guild-org/conductor-t2/conductor:b06169dcfdb7dc3a17844a91eda94e1b6031a1f3"
}
}

@dotansimha dotansimha merged commit 32bcc85 into interpolate-vars Nov 27, 2023
12 checks passed
dotansimha pushed a commit that referenced this pull request Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants