cloudfoundry / cf-java-client

Java Client Library for Cloud Foundry
Apache License 2.0
328 stars 318 forks source link

Add readiness health check support in client #1213

Open Yavor16 opened 9 months ago

linux-foundation-easycla[bot] commented 9 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

Yavor16 commented 9 months ago

Hi, @pivotal-david-osullivan, I saw that you have commited a lot of changes recently, so can you take a look at my PR.

pivotal-david-osullivan commented 9 months ago

@Yavor16 thank you for the contribution! We are working through PRs at the moment.

Could you please add integration test support to cover the new feature? Liveness health checks can be seen in the integration-test module in places such as here - test setting the type and here - setting the health check in an application push.

Yavor16 commented 9 months ago

Hi, @pivotal-david-osullivan, I can't write integration test and test it because I don't have my own cloudfoundry environment. The one I use for work, I don't have administrator access to it. If you want I can write a test but if you can run it.

Yavor16 commented 9 months ago

Hi, @pivotal-david-osullivan, I wrote an integration test and tested it

anthonydahanne commented 8 months ago

hello @Yavor16 , sorry to ask you this late, but could you run mvn spotless:apply and commit / amend ?

It will fix the CI failure on Java 11 that runs the stylecheck

Thank you! worst case, we'll do it ourselves

anthonydahanne commented 8 months ago

Hello @Yavor16 Unfortunately, after integration tests run, we had a few errors; such as:

[ERROR] org.cloudfoundry.client.v3.ProcessesTest.updateReadinessHealthCheckType
[ERROR]   Run 1: ProcessesTest.updateReadinessHealthCheckType expectation "expectNext(port)" failed (expected: onNext(port); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.applications.GetApplicationProcessResponse`, problem: Cannot build GetApplicationProcessResponse, some of required attributes are not set [readinessHealthCheck]

or

[ERROR] org.cloudfoundry.client.v3.ProcessesTest.update
[ERROR]   Run 1: ProcessesTest.update expectation "expectNext(process)" failed (expected: onNext(process); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.applications.GetApplicationProcessResponse`, problem: Cannot build GetApplicationProcessResponse, some of required attributes are not set [readinessHealthCheck]
 at [Source: (byte[])"{"guid":"edeef781-4335-43ac-9343-39f7409759c6","created_at":"2024-03-01T04:57:36Z","updated_at":"2024-03-01T04:57:45Z","type":"web","command":"$HOME/boot.sh","instances":1,"memory_in_mb":64,"disk_in_mb":258,"health_check":{"type":"port","data":{"timeout":null,"invocation_timeout":null}},"relationships":{"app":{"data":{"guid":"edeef781-4335-43ac-9343-39f7409759c6"}},"revision":{"data":{"guid":"a7a89e0e-ac15-43c4-87eb-9e21d3c5a785"}}},"metadata":{"labels":{},"annotations":{}},"links":{"self":{"hre"[truncated 559 bytes]; line: 1, column: 1059]))

or

[ERROR] org.cloudfoundry.operations.ApplicationsTest.pushManifestV3
[ERROR]   Run 1: ApplicationsTest.pushManifestV3 expectation "expectComplete" failed (expected: onComplete(); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.processes.ProcessResource`, problem: Cannot build ProcessResource, some of required attributes are not set [readinessHealthCheck]
 at [Source: (byte[])"{"pagination":{"total_results":1,"total_pages":1,"first":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/67ee72fe-8b93-40fc-80e4-7ce0f1c08e9b/processes?page=1\u0026per_page=50"},"last":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/67ee72fe-8b93-40fc-80e4-7ce0f1c08e9b/processes?page=1\u0026per_page=50"},"next":null,"previous":null},"resources":[{"guid":"67ee72fe-8b93-40fc-80e4-7ce0f1c08e9b","created_at":"2024-03-01T05:20:56Z","updated_at":"2024-03-01T05:21:09Z","type":"web","c"[truncated 944 bytes]; line: 1, column: 1442] (through reference chain: org.cloudfoundry.client.v3.applications.ListApplicationProcessesResponse$Json["resources"]->java.util.ArrayList[0])))

or

[ERROR] org.cloudfoundry.operations.ApplicationsTest.pushManifestV3MultipleApplications
[ERROR]   Run 1: ApplicationsTest.pushManifestV3MultipleApplications expectation "expectComplete" failed (expected: onComplete(); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Cannot construct instance of `org.cloudfoundry.client.v3.processes.ProcessResource`, problem: Cannot build ProcessResource, some of required attributes are not set [readinessHealthCheck]
 at [Source: (byte[])"{"pagination":{"total_results":1,"total_pages":1,"first":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/2a9c07ee-deb5-464e-a5f6-2875b0b30c45/processes?page=1\u0026per_page=50"},"last":{"href":"https://api.sys.polishedpine.cf-app.com/v3/apps/2a9c07ee-deb5-464e-a5f6-2875b0b30c45/processes?page=1\u0026per_page=50"},"next":null,"previous":null},"resources":[{"guid":"2a9c07ee-deb5-464e-a5f6-2875b0b30c45","created_at":"2024-03-01T05:11:27Z","updated_at":"2024-03-01T05:11:44Z","type":"web","c"[truncated 944 bytes]; line: 1, column: 1442] (through reference chain: org.cloudfoundry.client.v3.applications.ListApplicationProcessesResponse$Json["resources"]->java.util.ArrayList[0])))

Best guess is you missed a required JSON field named readinessHealthCheck

Yavor16 commented 8 months ago

Hello, @anthonydahanne, I've run the failing tests locally and all of them passed. Also I've run mvn spotless:apply but the file didn't change

Yavor16 commented 8 months ago

Here is a gist with the my test results https://gist.github.com/Yavor16/2d6b9c4786242c2bf1eb3ebebe5383da

anthonydahanne commented 7 months ago

hello @Yavor16 We got bad news: after rebasing and re testing your PR, we found out that your PR is relying on a recent CAPI version; much more recent than the default we target with current java-cf-client ; which is 2.186.0

We could merge this PR after we require a later version than 2.186.0

Thanks!

Yavor16 commented 7 months ago

Hello, @anthonydahanne So can we add a new feature to the client when the cersion you are using doesn't support it. Also how long do we have to wait for a new CAPI version.

beyhan commented 5 months ago

I know about users willing to use the readiness health check blocked by this pr. From the platform side we are also interested to get more feedback for new features. It will be great if the CF Java client could support more recent versions of CAPI which will help also for the adoption of those new features. @pivotal-david-osullivan, @anthonydahanne is there a chance we find a good middle ground here?

anthonydahanne commented 5 months ago

@beyhan , yes, we want to unblock this situation. we'll update you soon

boyan-velinov commented 3 months ago

@anthonydahanne Do you have some news on that? Did you establish a plan how to move the change forward? Users are eager to use readiness health check feature