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

api-server-rest: Add extend_runtime_measurement API #635

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arronwy
Copy link
Member

@arronwy arronwy commented Jul 31, 2024

No description provided.

@arronwy arronwy requested a review from sameo as a code owner July 31, 2024 06:52
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

nice. I prefer to have some spec before this. wdyt?

(status = 200, description = "runtime measurement extend success",
content_type = "application/octet-stream",
body = String,
example = json!({"domain":"image-rs","operation":"CreateContainer", "content":"docker.io/library/alpine", "register_index":"17"})),
Copy link
Member

Choose a reason for hiding this comment

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

let's update the example when the spec is finished.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we'll update this part to follow the spec.

(Some(domain), Some(operation), Some(content)) => {
let register_index: Option<u64> = params
.get("register_index")
.and_then(|value| value.parse::<u64>().ok());
Copy link
Member

Choose a reason for hiding this comment

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

There is a case where register_index is given by caller but failed to be parsed. The register_index will be None thus inside AA a default one. The caller should know about this, or return an error when parsing fails. wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree, return error is make sense for me, I'll update this part.

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.

2 participants