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

Extract Config from Tracker and Heath Check #623

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

da2ce7
Copy link
Contributor

@da2ce7 da2ce7 commented Jan 17, 2024

This is the first three commits from #557

It includes an significant improvement of the Heath Check logic, as it now takes the active port from the binding, instead of the configuration.

@da2ce7 da2ce7 mentioned this pull request Jan 17, 2024
5 tasks
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (dde3d8d) 77.16% compared to head (3f0dcea) 77.62%.

Files Patch % Lines
src/app.rs 0.00% 14 Missing ⚠️
src/core/mod.rs 80.00% 7 Missing ⚠️
src/servers/health_check_api/resources.rs 66.66% 7 Missing ⚠️
src/servers/registar.rs 78.78% 7 Missing ⚠️
src/bootstrap/jobs/health_check_api.rs 0.00% 4 Missing ⚠️
src/bootstrap/app.rs 66.66% 2 Missing ⚠️
src/bootstrap/jobs/udp_tracker.rs 0.00% 2 Missing ⚠️
src/bootstrap/jobs/torrent_cleanup.rs 0.00% 1 Missing ⚠️
src/main.rs 0.00% 1 Missing ⚠️
src/shared/bit_torrent/tracker/udp/client.rs 92.30% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #623      +/-   ##
===========================================
+ Coverage    77.16%   77.62%   +0.45%     
===========================================
  Files          124      125       +1     
  Lines         8242     8334      +92     
===========================================
+ Hits          6360     6469     +109     
+ Misses        1882     1865      -17     

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

josecelano
josecelano previously approved these changes Jan 17, 2024
Copy link
Member

@josecelano josecelano left a comment

Choose a reason for hiding this comment

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

Wonderful job @da2ce7! I've only added some comments for things that were "harder" to read for me. I mean, I had to read the details to understand what they do, but only in a few cases and it was fast to get on track. So it's perfectly mergeable.

packages/configuration/src/lib.rs Show resolved Hide resolved
packages/test-helpers/src/configuration.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
@josecelano josecelano added the Needs Rebase Base Branch has Incompatibilities label Jan 17, 2024
@da2ce7 da2ce7 force-pushed the 20240117_cleanup_health_check branch from fda6f41 to 3f0dcea Compare January 17, 2024 15:21
@da2ce7 da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Jan 17, 2024
@josecelano
Copy link
Member

ACK 3f0dcea

@josecelano josecelano merged commit 203ce96 into torrust:develop Jan 17, 2024
12 checks passed
@josecelano
Copy link
Member

Hi @da2ce7 after merging this PR there are some new problems running the services.

I want to add E2E tests to avoid this type of regression.

@josecelano josecelano mentioned this pull request Jan 19, 2024
1 task
josecelano added a commit that referenced this pull request Jan 19, 2024
17296cd fix: [#626] healt check api server shutdown (Jose Celano)
bbf1be6 fix: don't start HTTP tracker if it's disabled (Jose Celano)

Pull request description:

  This fixes all errors introduced after merging [this PR](#623).

  - [x] HTTP tracker should not be started if it's disabled

  Error running the Health Check API server:

  ```console
  thread 'tokio-runtime-worker' panicked at src/servers/signals.rs:52:25:
  Failed to install stop signal: channel closed
  ```

ACKs for top commit:
  josecelano:
    ACK 17296cd

Tree-SHA512: 95fb8011f6eb58b52f8805185ccabb484be29d6231930aaa9a24d4a9b0a4b5dd457259a343c4a337529264ecd42f76609a57914a661a7ce19255e0fcb41a3145
@josecelano josecelano linked an issue Jan 26, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Fix container HEALTHCHECK when port 0 is used in confguration
2 participants