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

Todo #3636

Closed
Closed

Conversation

StonewallJohnson
Copy link

Changes

  • Added doc string to Endpoint::new()
  • Found better name for app_metrics

Reason

Resolve TODOs according to #3273

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • ☑ If a specific issue led to this PR, this PR closes the issue.
  • ☑ The description of changes is clear and encompassing.
  • ☑ Any required documentation changes (code and docs) are included in this PR.
  • ☑ API changes follow the Runbook for Firecracker API changes.
  • ☑ User-facing changes are mentioned in CHANGELOG.md.
  • ☑ All added/changed functionality is tested.
  • ☑ New TODOs link to an issue.
  • ☑ Commits meet contribution quality standards.

  • This functionality cannot be added in rust-vmm.

roypat
roypat previously requested changes May 30, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Mhh, it looks like you put the pull request description into the template file, instead of just filing it out on GitHub 🤔 Could you drop this file from your PR, please?

@@ -87,18 +87,16 @@ pub struct Metrics<T: Serialize> {
// Metrics will get flushed here.
metrics_buf: Mutex<Option<Box<dyn Write + Send>>>,
is_initialized: AtomicBool,
pub app_metrics: T,
pub metrics_buf_input: T,
Copy link
Contributor

Choose a reason for hiding this comment

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

Mhh, the only reason this name makes sense is because of the todo-comment, which this PR removes. Maybe metrics_data makes more sense?

@@ -79,13 +79,13 @@ pub struct Endpoint {
// is the only option).

impl Endpoint {
/// Asserts that RCV_BUF_MAX_SIZE <= MAX_WINDOW_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead write a complete doc comment here with a ## Panics: section that mentions this simplifying assumption?

Landon Johnson added 2 commits May 31, 2023 21:00
Signed-off-by: Landon Johnson <landon@cs.utexas.edu>
Signed-off-by: Landon Johnson <landon@cs.utexas.edu>
StonewallJohnson and others added 3 commits June 11, 2023 15:17
Signed-off-by: Landon Johnson <landon@cs.utexas.edu>
Signed-off-by: Landon Johnson <landon@cs.utexas.edu>
@roypat roypat dismissed their stale review June 26, 2023 09:21

no more template modifications

@xmarcalx
Copy link
Contributor

xmarcalx commented Aug 7, 2023

Hi @StonewallJohnson ,

Thanks for your contribution.
Are you still following this PR? Can you address @roypat comments?

roypat added a commit to roypat/firecracker that referenced this pull request May 8, 2024
Resolve a TODO about documenting a panic condition.

Closes firecracker-microvm#3636

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Co-Authored-by: Landon Johnson <landon@cs.utexas.edu>
roypat added a commit to roypat/firecracker that referenced this pull request May 8, 2024
The name is fine. When it was tried to be changed in firecracker-microvm#3636 we could not
come up with anything better really.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat added a commit to roypat/firecracker that referenced this pull request May 8, 2024
Resolve a TODO about documenting a panic condition.

Closes firecracker-microvm#3636

Co-Authored-by: Landon Johnson <landon@cs.utexas.edu>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat added a commit to roypat/firecracker that referenced this pull request May 8, 2024
The name is fine. When it was tried to be changed in firecracker-microvm#3636 we could not
come up with anything better really.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat closed this in e08e39a May 14, 2024
roypat added a commit that referenced this pull request May 14, 2024
The name is fine. When it was tried to be changed in #3636 we could not
come up with anything better really.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
brandonpike pushed a commit to brandonpike/firecracker that referenced this pull request May 20, 2024
Resolve a TODO about documenting a panic condition.

Closes firecracker-microvm#3636

Co-Authored-by: Landon Johnson <landon@cs.utexas.edu>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
brandonpike pushed a commit to brandonpike/firecracker that referenced this pull request May 20, 2024
The name is fine. When it was tried to be changed in firecracker-microvm#3636 we could not
come up with anything better really.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
brandonpike pushed a commit to brandonpike/firecracker that referenced this pull request May 20, 2024
Resolve a TODO about documenting a panic condition.

Closes firecracker-microvm#3636

Co-Authored-by: Landon Johnson <landon@cs.utexas.edu>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
brandonpike pushed a commit to brandonpike/firecracker that referenced this pull request May 20, 2024
The name is fine. When it was tried to be changed in firecracker-microvm#3636 we could not
come up with anything better really.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
brandonpike pushed a commit to brandonpike/firecracker that referenced this pull request May 20, 2024
Resolve a TODO about documenting a panic condition.

Closes firecracker-microvm#3636

Co-Authored-by: Landon Johnson <landon@cs.utexas.edu>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
brandonpike pushed a commit to brandonpike/firecracker that referenced this pull request May 20, 2024
The name is fine. When it was tried to be changed in firecracker-microvm#3636 we could not
come up with anything better really.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
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.

None yet

3 participants