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

DSP-743 Support the api route /health #246

Merged
merged 14 commits into from
Oct 12, 2020
Merged

Conversation

kilchenmann
Copy link
Contributor

resolves DSP-743

@kilchenmann kilchenmann self-assigned this Oct 8, 2020
@kilchenmann kilchenmann marked this pull request as draft October 8, 2020 06:38
@kilchenmann kilchenmann added the enhancement Improve existing code or new feature label Oct 8, 2020
@tobiasschweizer
Copy link
Contributor

@kilchenmann It is possible to get the headers from an RxJS ajax get request.

    getHealth(): Observable<ApiResponseData<HealthResponse> | ApiResponseError> {
    
        return this.httpGet("").pipe(
            tap(res => console.log(res.xhr.getResponseHeader("server"))),
            map(ajaxResponse => ApiResponseData.fromAjaxResponse(ajaxResponse, HealthResponse, this.jsonConvert)),
            catchError(error => this.handleError(error))
        );
    
    }

I just print out the header using tap: "webapi/v13.0.0-rc.16-11-ga88d20d akka-http/10.1.12"

@tobiasschweizer
Copy link
Contributor

@kilchenmann I added the contents to the test data file in 64c159d

@kilchenmann
Copy link
Contributor Author

@tobiasschweizer Would it be possible to add it to the response, or how would I read it in the app?

@tobiasschweizer
Copy link
Contributor

Do you already have some logic to parse the server header param?

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Oct 8, 2020

@tobiasschweizer Would it be possible to add it to the response, or how would I read it in the app?

I think we could add the server header param to HealthResponse. This could be handled in the getHealth method after deserialising it using json2typescript.

If you want, I could work on this.

@kilchenmann
Copy link
Contributor Author

Do you already have some logic to parse the server header param?

No not yet

@kilchenmann
Copy link
Contributor Author

@tobiasschweizer Would it be possible to add it to the response, or how would I read it in the app?

I think we could add the server header param to HealthResponse. This could be handled in the getHealth method after the deserialising it using json2typescript.

Yes, this was my idea. Would be great if you can work on it. Thanks

@tobiasschweizer
Copy link
Contributor

@kilchenmann 7b890c5 adds functionality to parse the header and add it to the health message. I will probably this in a utility file, so it can be called from the endpoint. Also I will add tests that check the error handling works as expected.

@tobiasschweizer
Copy link
Contributor

@kilchenmann I think we should make a DSP task so we can get test data for the health response message. I don't know how we can get test data for the headers.

@tobiasschweizer
Copy link
Contributor

tobiasschweizer commented Oct 9, 2020

I created https://dasch.myjetbrains.com/youtrack/issue/DSP-816 so Knora generates test data for the health route.

@kilchenmann
Copy link
Contributor Author

I created https://dasch.myjetbrains.com/youtrack/issue/DSP-816 so Knora generates test data for the health route.

Do we have to wait for it? Or can we update the tests in a later PR?

@tobiasschweizer
Copy link
Contributor

I created https://dasch.myjetbrains.com/youtrack/issue/DSP-816 so Knora generates test data for the health route.

Do we have to wait for it? Or can we update the tests in a later PR?

I think we'll have to wait. I created https://dasch.myjetbrains.com/youtrack/issue/DSP-817.

@tobiasschweizer
Copy link
Contributor

Ok, I will check this PR this morning.

@kilchenmann
Copy link
Contributor Author

I am not allowed to review my PR myself. So, I approve the changes from @tobiasschweizer: It works well

@kilchenmann kilchenmann marked this pull request as ready for review October 12, 2020 09:48
@tobiasschweizer
Copy link
Contributor

ok, the PR is good to go!

@tobiasschweizer tobiasschweizer merged commit 3649730 into master Oct 12, 2020
@tobiasschweizer tobiasschweizer deleted the wip/dsp-743-health-route branch October 12, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants