feat: s3 health check

[elbiocaetano]

No description provided.

[github-actions]

Next minor release will be 4.5.0, Show Release Notes

4.5.0 (2024-05-30)

Features

• s3 health check (19b174e)

[github-actions]

Test Results

  234 files  +2    234 suites  +2   8m 32s ⏱️ +24s
1 150 tests  - 1  1 142 ✅  - 1   8 💤 ±0  0 ❌ ±0 
2 336 runs  +6  2 316 ✅ +6  20 💤 ±0  0 ❌ ±0 

Results for commit 19b174e. ± Comparison against base commit 662ff2a.

This pull request removes 4 and adds 3 tests. Note that renamed tests count towards both.

org.sdase.commons.spring.boot.metadata.context.MetadataContextMergeIntegrationTest ‑ [4] {processes=[p-c1, p-c2], tenant-id=[t-c]}, {processes=[p-n3, p-n4], tenant-id=[]}, EXTEND, {processes=[p-c1, p-c2, p-n3, p-n4], tenant-id=[t-c]}
org.sdase.commons.spring.boot.metadata.context.MetadataContextMergeIntegrationTest ‑ [5] {tenant-id=[t-c]}, {processes=[p-n]}, EXTEND, {processes=[p-n], tenant-id=[t-c]}
org.sdase.commons.spring.boot.metadata.context.MetadataContextMergeIntegrationTest ‑ [8] {tenant-id=[t-c]}, {processes=[p-n], tenant-id=[t-n]}, KEEP, {processes=[p-n], tenant-id=[t-c]}
org.sdase.commons.spring.boot.metadata.context.MetadataContextMergeIntegrationTest ‑ [9] {processes=[p-c], tenant-id=[t-c]}, {tenant-id=[t-n]}, REPLACE, {processes=[p-c], tenant-id=[t-n]}

org.sdase.commons.spring.boot.s3.config.S3HealthIndicatorIntegrationTest ‑ checkThatS3HealthCheckIsEnabledAndDown()
org.sdase.commons.spring.boot.s3.config.S3HealthIndicatorIntegrationTest ‑ checkThatS3HealthCheckIsEnabledAndUp()
org.sdase.commons.spring.boot.s3.config.S3HealthIndicatorIntegrationTest ‑ checkThatS3HealthCheckIsNotEnabled()

♻️ This comment has been updated with latest results.

[JoergSiebahn]

Just some comments.

[JoergSiebahn]

NIT: Imho this can be simplified, as LOGGER.error will also suppress the message if not enabled (and error is likely always enabled). I would also go for a custom message and attach the exception, e.g. like this:

Suggested change

	if (LOGGER.isErrorEnabled()) {
	LOGGER.error(e.getMessage(), e);
	}
	LOGGER.error("S3 health check failed to get info of bucket {}", bucketName, e);

[JoergSiebahn]

As the health check is propagating to the health endpoint anyways, I wonder if the log could also be a warning.

[JoergSiebahn]

NIT: As this is a SpringBootTest, isn't it possible to set it as property?

Suggested change

	@SetSystemProperty(key = "management.health.s3.enabled", value = "true")
	@SpringBootTest(
	classes = S3TestApp.class,
	webEnvironment = WebEnvironment.RANDOM_PORT,
	properties = {"auth.disable=true", "opa.disable=true", "management.server.port=8071"})
	@SpringBootTest(
	classes = S3TestApp.class,
	webEnvironment = WebEnvironment.RANDOM_PORT,
	properties = {"auth.disable=true", "opa.disable=true", "management.server.port=8071", "management.health.s3.enabled=true"})

[elbiocaetano]

Unfortunately no, because otherrwise it would ovewrite the @SetSystemProperty in the checkThatS3HealthCheckIsNotEnabled test.

[JoergSiebahn]

NIT: Can the dirty context be avoided if we don't use SetSystemProperty? Afaik, this slows down the test suite.

[elbiocaetano]

The problem is that I want to test when it is enabled and when it is not, so I need to run the server again for each test. In order to do that I either use this approach or I separate in two test classes, one with the health check enabled, and the other one disabled.