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

Heimdallr v2 #41

Merged
merged 22 commits into from
Sep 14, 2023
Merged

Heimdallr v2 #41

merged 22 commits into from
Sep 14, 2023

Conversation

styvane
Copy link
Collaborator

@styvane styvane commented Aug 31, 2023

This PR is an alternate implementation for the Heimdallr service.

It addresses the following issues while keeping existing features.

  • Improve configuration management
  • Keep Secret secret
  • Better Error handling
  • Migrate from Warp to Axum to simplify HTTP handlers and routes.
  • Additional minor improvements

@styvane styvane requested review from Sbcdn and amab8901 and removed request for Sbcdn August 31, 2023 07:46
@Sbcdn
Copy link
Owner

Sbcdn commented Sep 8, 2023

Hey @styvane ,

First of all: Good Job! :)

Is a nice service, better structured and documented as the v1 in any case!
The advantage of axum over warp is for me still a taste thingy up to now :)
One thing which would be a big plus would be an automatic swagger for api documentation, does this exist for axum?
For warp there is rweb which tries to plug swagger on top of warp but it works medium, you have an idea?

There is a conflict on the "regex-syntax" version can we just use the latest (i think 7.5) or is there a dependency issue to 6.29 ?

Copy link
Owner

@Sbcdn Sbcdn left a comment

Choose a reason for hiding this comment

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

Nice!

Axum seems to lean on nest-js architecture, no ?

services/heimdallr-v2/src/routes/contract.rs Show resolved Hide resolved
services/heimdallr-v2/src/settings.rs Show resolved Hide resolved
services/heimdallr-v2/src/settings.rs Show resolved Hide resolved
@amab8901
Copy link
Contributor

amab8901 commented Sep 8, 2023

I like the way you documented everything 👍 what about unit tests?

@styvane
Copy link
Collaborator Author

styvane commented Sep 11, 2023

Hey @styvane ,

First of all: Good Job! :)

Is a nice service, better structured and documented as the v1 in any case! The advantage of axum over warp is for me still a taste thingy up to now :) One thing which would be a big plus would be an automatic swagger for api documentation, does this exist for axum? For warp there is rweb which tries to plug swagger on top of warp but it works medium, you have an idea?

There is a conflict on the "regex-syntax" version can we just use the latest (i think 7.5) or is there a dependency issue to 6.29 ?

Thank you for the feedback, Axum does not have a way to automatically generate OpenAPI documentation at the time of this writing see tokio-rs/axum#50 . Maybe we can utoipa for that.

There is a dependency in the chain that depends on regex-syntax 6.29 that is why. We don't use regex-syntax explicitly

@styvane
Copy link
Collaborator Author

styvane commented Sep 11, 2023

I like the way you documented everything 👍 what about unit tests?

Thank you for looking into it, and you're right about testing.

Regarding the API testing, we are currently not injecting a client into the handler; we can do that by having something like a ClientService trait for odin that we will implement and use in our TestApp This will automatically take care of any I/O issue for testing.

For the API tests, look into the tests/api module.

I will unit-test the features that do not rely on the test client.

@styvane
Copy link
Collaborator Author

styvane commented Sep 11, 2023

@amab8901 I've add a test for the Claims extractor and fixed a bug in the process. See 8d57a5d Thank you!

@styvane styvane merged commit 13bb5b7 into main Sep 14, 2023
@styvane styvane deleted the heimdallr-v2 branch September 14, 2023 09:24
Sbcdn pushed a commit that referenced this pull request Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants