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

Add comments to main.rs #38

Merged
merged 2 commits into from
May 27, 2021
Merged

Add comments to main.rs #38

merged 2 commits into from
May 27, 2021

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented May 26, 2021

Start for #14
I have only commented main.rs, so you can check the commenting style, and let me know if you have something different in mind. If this is fine, after this I'll make a PR of 2-3 files at once as you said in #14.
I have also started a high-level outerview documentation, which will help understand the overall flow and components. I think as I am writing this not knowing much about container runtimes or youki, this will be helpful for new contributors.
Let me know if any changes are required.

Start draft of high-level documentation
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

@YJDoc2
Your work is inspiring to me! The comments and documentation will be great for developers, including new contributors. The comments will also greatly increase the likelihood of youki being used as a crate and the comfort of using it.
Have you ever heard of the broken windows theory? The current youki has broken windows due to my failure to comment. However, if you comment, the window will be repaired and cleaned up, and the culture will be such that other developers will also leave comments.
The culture of comments you create will be wonderful.

This is my mistake; a high-level container runtime that includes docker might be appropriate, not just docker, except for the control flow diagram.

@utam0k utam0k self-requested a review May 26, 2021 23:47
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented May 27, 2021

Hey, Thanks!
Can you tell exactly where the changes are required? I kept docker in the flow diagram as it it not final anyways, and in comments I only mentioned docker, as I think currently it is only tested on docker.
Let me know what changes are required and I will update the PR.

src/main.rs Outdated
@@ -66,6 +78,8 @@ impl SubCommand {
}
}

/// This is the entry point in the container runtime. The binary is run by docker,
Copy link
Member

Choose a reason for hiding this comment

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

I'd like you to change docker to the high-level container runtime.

src/main.rs Outdated
@@ -1,3 +1,7 @@
//! # Youki
//! Container Runtime written in Rust, inspired by [railcar](https://github.com/oracle/railcar)
//! This crate provides a container runtime which can be used by Docker to run containers.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like you to change docker to the high-level container runtime.

@utam0k
Copy link
Member

utam0k commented May 27, 2021

@YJDoc2 I apologize for the confusing explanation.
I have commented on it. Basically, if you don't show a concrete example (e.g. flow diagram, etc.), I hope you will use the term higher-level container runtime instead of docker.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented May 27, 2021

@utam0k
Hey, I have made the requested changes, please review.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

nice work! I'm looking forward next to your PR.

@utam0k utam0k merged commit 1e4b4e8 into youki-dev:main May 27, 2021
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