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

Updates for health contributors and endpoint #1451

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

TimHess
Copy link
Member

@TimHess TimHess commented Jan 30, 2025

Description

Updates to more closely align with Spring:

  • remove (duplicated) status from details
  • improve probe parity with Spring
  • separate switches for showing components and their details
  • enhance groups to override showComponents and showDetails
  • add path & exists property to disk space

Resolves #1440

docs updated in SteeltoeOSS/Documentation#343

Open question to resolve: Should liveness and readiness be controlled by a shared switch like management:endpoint:health:probes:enabled, as they are in Spring? I think probably not since we're already at least slightly mismatched by using "endpoints" instead of "endpoint", but I'm open to making the change

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.

Base automatically changed from remove_negotiation to main January 30, 2025 17:25
@TimHess TimHess self-assigned this Jan 30, 2025
@TimHess TimHess added this to the 4.0.0-m1 milestone Jan 30, 2025
@TimHess TimHess added Component/Management Issues related to Steeltoe Management (actuators) ReleaseLine/4.x Identified as a feature/fix for the 4.x release line Warn/breaking-change There is a breaking change in the code labels Jan 30, 2025
@TimHess TimHess marked this pull request as ready for review January 30, 2025 19:52
Copy link
Member

@bart-vmware bart-vmware left a comment

Choose a reason for hiding this comment

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

Reviewed a subset of the changes.

@@ -153,10 +153,12 @@ private async Task ActAndAssertAsync(IHostBuilder builder, Type endpointOptionsT
response.StatusCode.Should().Be(expectSuccess ? HttpStatusCode.OK : HttpStatusCode.Forbidden);
}

#pragma warning disable SA1602 // Enumeration items should be documented
Copy link
Member

Choose a reason for hiding this comment

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

Why was this added? It wasn't needed before, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it because I was getting SA1602 errors here. I thought that was odd, but didn't think it was worth trying to understand or document

Copy link
Member

Choose a reason for hiding this comment

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

Where did you get the errors exactly? I see them in VS, but running a build in release mode (both from VS and dotnet build) doesn't produce errors.

image

Either way, it's pretty unusual to have public enums in test projects. However, in this case, it's required for TheoryData. A better way to suppress is to add SA1602 to NoWarn in shared-test.props. Because we'd never want errors in test projects for that.

@bart-vmware

This comment was marked as resolved.

TimHess and others added 3 commits January 31, 2025 09:26
- remove status from details
- improve probe parity with Spring
- separate switches for showing components and their details
- enhance groups to override showComponents and showDetails
- add path & exists property to disk space
@TimHess

This comment was marked as resolved.

@TimHess TimHess requested a review from bart-vmware January 31, 2025 16:31
@@ -8,20 +8,20 @@
namespace Steeltoe.Management.Endpoint.Actuators.Health;

internal sealed class PostConfigureHealthEndpointOptions(
IOptions<LivenessStateContributorOptions> livenessOptions, IOptions<ReadinessStateContributorOptions> readinessOptions)
IOptionsMonitor<LivenessStateContributorOptions> livenessOptions, IOptionsMonitor<ReadinessStateContributorOptions> readinessOptions)
Copy link
Member

Choose a reason for hiding this comment

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

parameters should be renamed to livenessOptionsMonitor/readinessOptionsMonitor, as their types have changed. And please introduce private readonly fields instead of accessing the primary ctor parameters directly.

@@ -74,17 +81,4 @@ public void Constructor_BindsRoleCorrectly()
Assert.Equal(ClaimTypes.Role, options.Claim.Type);
Assert.Equal("role-claim-value", options.Claim.Value);
}

[Fact]
public void CanClearDefaultGroups()
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand, readiness/liveness contributors are not added automatically anymore. However, when they are added, their groups are automatically created. We still need a way to configure not creating any groups.

@@ -153,10 +153,12 @@ private async Task ActAndAssertAsync(IHostBuilder builder, Type endpointOptionsT
response.StatusCode.Should().Be(expectSuccess ? HttpStatusCode.OK : HttpStatusCode.Forbidden);
}

#pragma warning disable SA1602 // Enumeration items should be documented
Copy link
Member

Choose a reason for hiding this comment

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

Where did you get the errors exactly? I see them in VS, but running a build in release mode (both from VS and dotnet build) doesn't produce errors.

image

Either way, it's pretty unusual to have public enums in test projects. However, in this case, it's required for TheoryData. A better way to suppress is to add SA1602 to NoWarn in shared-test.props. Because we'd never want errors in test projects for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component/Management Issues related to Steeltoe Management (actuators) ReleaseLine/4.x Identified as a feature/fix for the 4.x release line Warn/breaking-change There is a breaking change in the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit Liveness, Readiness and health groups
2 participants