Skip to content

Consul :: Add tests to cover health check path being used when heartbeat is off#1245

Merged
TimHess merged 3 commits into
SteeltoeOSS:release/3.2from
cieciurm:fix/health-check-path
Jan 18, 2024
Merged

Consul :: Add tests to cover health check path being used when heartbeat is off#1245
TimHess merged 3 commits into
SteeltoeOSS:release/3.2from
cieciurm:fix/health-check-path

Conversation

@cieciurm
Copy link
Copy Markdown
Contributor

@cieciurm cieciurm commented Jan 11, 2024

Description

Hi all,

This PR fixes an issue when health check path is not respected when heartbeat is enabled.

This is a very simple attempt to fix the issue, by moving the short circuiting statement couple of lines lower.
Please let me know if it's too simple ;)
I've added unit tests to cover the issue scenario.

As a bonus, I've added a unit test to assert an exception being thrown when the specified port is a negative number.

If the solution is approved, then I can submit a similar fix for the v4 version (targeting main branch)

Fixes #1062

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

- Add tests for heartbeat on/off
- Add tests for negative port
@cieciurm cieciurm force-pushed the fix/health-check-path branch from d27f8f2 to 991347c Compare January 11, 2024 14:28
Comment thread src/Discovery/src/Consul/Registry/ConsulRegistration.cs Outdated
Comment thread src/Discovery/src/Consul/Registry/ConsulRegistration.cs Outdated
@TimHess TimHess added ReleaseLine/3.x Identified as a feature/fix for the 3.x release line Component/Discovery Issues related to Steeltoe Service Discovery labels Jan 17, 2024
@TimHess TimHess added this to the 3.2.7 milestone Jan 17, 2024
@cieciurm cieciurm changed the title Consul :: Fix health check path is not working when heartbeat is on Consul :: Add tests to cover health check path being used when heartbeat is off Jan 18, 2024
Copy link
Copy Markdown
Member

@TimHess TimHess left a comment

Choose a reason for hiding this comment

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

LGTM

@TimHess
Copy link
Copy Markdown
Member

TimHess commented Jan 18, 2024

/azp run Steeltoe.All

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@TimHess
Copy link
Copy Markdown
Member

TimHess commented Jan 18, 2024

CI failures are unrelated

@TimHess TimHess merged commit dc772e3 into SteeltoeOSS:release/3.2 Jan 18, 2024
@cieciurm cieciurm deleted the fix/health-check-path branch January 20, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component/Discovery Issues related to Steeltoe Service Discovery ReleaseLine/3.x Identified as a feature/fix for the 3.x release line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants