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

Make the API configuration modular #861

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

ansasaki
Copy link
Contributor

Move the configuration of the API for each scope into the respective handler file.
Also, move the API configuration into a dedicated file, and make it modular.

This is a preparation to add support for multiple API versions.

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 72.94118% with 69 lines in your changes missing coverage. Please review.

Project coverage is 60.01%. Comparing base (2f7b3ad) to head (9e528f1).
Report is 63 commits behind head on master.

Files with missing lines Patch % Lines
keylime-agent/src/api.rs 70.14% 20 Missing ⚠️
keylime-agent/src/keys_handler.rs 64.44% 16 Missing ⚠️
keylime-agent/src/agent_handler.rs 63.33% 11 Missing ⚠️
keylime-agent/src/notifications_handler.rs 67.64% 11 Missing ⚠️
keylime-agent/src/quotes_handler.rs 65.62% 11 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 60.01% <72.94%> (+2.42%) ⬆️
upstream-unit-tests 60.01% <72.94%> (+9.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
keylime-agent/src/errors_handler.rs 55.81% <ø> (-0.35%) ⬇️
keylime-agent/src/main.rs 21.33% <100.00%> (-4.69%) ⬇️
keylime-agent/src/agent_handler.rs 59.57% <63.33%> (ø)
keylime-agent/src/notifications_handler.rs 66.66% <67.64%> (ø)
keylime-agent/src/quotes_handler.rs 52.29% <65.62%> (+1.42%) ⬆️
keylime-agent/src/keys_handler.rs 64.97% <64.44%> (-0.47%) ⬇️
keylime-agent/src/api.rs 70.14% <70.14%> (ø)

... and 8 files with indirect coverage changes

Copy link
Member

@THS-on THS-on left a comment

Choose a reason for hiding this comment

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

Generally LGTM, just would like to have someone else that has more experince with actix to also take a look at it.

Move the /keys scope configuration from main to the keys_handler module.
This is a preparation to support multiple API versions.

Also, make the methods that are not required outside of the module
private.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Move the /quotes scope configuration from main to quotes_handler module.
This is a preparation to support multiple API versions.

Also, make the methods that are not required outside the module private.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Move the /notifications scope configuration from main to
notifications_handler.  This is a preparation to support multiple API
versions.

Also, restrict the visibility of methods that are not required outside
the module to private.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Move the /agent scope configuration from main to agent_handler.
This is a preparation to support multiple API versions.

Also, restrict the visibility of methods that are not required outside
the module to private.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Move the API configuration to a dedicated file and make it modular.

The API configuration is separated for each supported version.
Currently only the latest API version (v2.2) is supported.

This is a preparation to support multiple API versions.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
Copy link
Contributor Author

ansasaki commented Nov 4, 2024

/packit retest-failed

@ansasaki
Copy link
Contributor Author

@ashcrow @mpeters @ueno @sergio-correia @aplanas Could someone please review/approve this? This PR is sitting for a month waiting for approval.

We definitely need more Rust developers involved.

Copy link
Contributor

@aplanas aplanas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sergio-correia sergio-correia left a comment

Choose a reason for hiding this comment

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

This looks good to me as well, thanks.

@ansasaki ansasaki merged commit 922e36b into keylime:master Nov 13, 2024
11 checks passed
@ansasaki ansasaki deleted the modular_api_config branch November 13, 2024 13:32
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.

7 participants