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

Networking related docs update #150

Merged
merged 16 commits into from
Aug 21, 2023
Merged

Networking related docs update #150

merged 16 commits into from
Aug 21, 2023

Conversation

juntao
Copy link
Member

@juntao juntao commented Aug 19, 2023

Explanation

Improve the organization and instructions how to best use networking libraries in Rust and JS. Specifically will address HTTPS and TLS use cases.

Improve wording and links.

Signed-off-by: Michael Yuan <[email protected]>
Copy link
Collaborator

alabulei1 commented Aug 19, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall summary:

The majority of the patches in this pull request focus on improving the clarity, organization, and correctness of the networking-related documentation. There are several potential issues and errors identified, including missing anchor texts for links, duplicated information, incomplete descriptions of changes, lack of context or reasoning for certain modifications, and potential compatibility or functionality differences with command replacements.

The most important findings are related to the changes in list style, the clarification of asynchronous and non-blocking behavior, and the modification of code snippets for proper command usage.

To improve the pull request, it is recommended to provide more specific details about the changes made within each section, address the potential problems and errors mentioned, and ensure that the modifications align with the project's requirements and provide sufficient context and explanation for users. Additionally, validating the changes through tests or verification processes is suggested to ensure accuracy and completeness.

Details

Commit 174c685ded26ce0db06b7bc1aad3f9b0a9119951

Key changes in this patch include:

  1. Improved wording and links in the document.
  2. Update of the section titles to make them clearer.
  3. Removal of unnecessary information.
  4. Reorganization of the content.

Potential problems:

  1. Some links to external resources are missing proper anchor texts, which can make it harder for users to understand the purpose of the links.
  2. The note about installing the WasmEdge TLS plug-in seems to be duplicated for the http_req and reqwest sections. It should be clarified whether it applies to both sections or only to a specific one.

Commit 2a29cc91586022d658231e281d99da91a94fc426

Key changes:

  • The duplicate TLS note has been removed.

Potential problems:

  • None of the changes in this patch introduce any potential problems.

Commit b5ca41a8fa2a199aa9e92154bed1b68f889743d1

Key changes:

  • Improved writing and layout of the document.
  • Clarified that the HTTP server is always asynchronous and non-blocking to handle concurrent requests.
  • Added links to the sections covering the warp API and the hyper API.
  • Updated the import statements for the warp and hyper crates.

Potential problems:

  • None of the changes seem to introduce problems. The changes are mainly focused on enhancing the clarity and organization of the document.

Commit 76ffb61442248cf0454bbbeca84082992a3dd41b

Key changes:

  • Fixed a typo in the document.

Potential problems:

  • No potential problems are identified in this patch.

Commit 7ddfb144738d9c107137384257f5c324cccc2f2c

Key changes in the patch:

  • Changed the list style for the sections on the warp API and the hyper API in the server.md file.

The most important finding is the change in list style, from an unordered list (*) to an ordered list (-), for the sections on the warp API and the hyper API.

Potential problems:

  • None apparent in this patch.

Commit a3806ec99f4a36eb4f8458a916cc18518f7c4290

Key changes in this patch:

  1. The title of the section has been changed from "Client" to "Socket client".
  2. The introductory paragraph has been updated to mention that WasmEdge applications can open TCP/IP or UDP network sockets in the host system.
  3. The link to the wasmedge_wasi_socket crate has been moved to be mentioned after the description of non-blocking sockets.
  4. The chapter now focuses on building HTTP clients on TCP sockets, and recommends checking out the "HTTP client" chapter for production HTTP clients.

Potential problems:

  1. The patch does not mention any updates to the content of the section, only the reorganization and introduction changes are mentioned. It is unclear from the patch what specific changes were made within each of the sub-sections ("A simple HTTP client based on TCP sockets" and "A non-blocking HTTP client based on TCP sockets").
  2. The link to the "HTTP client" chapter is mentioned, but it is not clear if the content of that chapter has been updated or if it is still relevant to this section.

Overall, it seems that the patch focuses on reorganizing and clarifying the introduction of the section, but more information is needed to understand the complete set of changes made.

Commit ed67c42a63283bd2b805413b77390603ab5602a3

Key changes in the patch:

  1. The title of the chapter has been updated from "Server" to "Socket server".
  2. The introductory paragraph has been modified to clarify that the WasmEdge socket API allows Rust developers to work directly on the TCP and UDP socket level.
  3. A link to the "HTTP server" chapter has been added for users seeking a production-ready HTTP server implementation.
  4. The code snippets have been updated to use the "wasmedge compile" command instead of "wasmedgec".

Potential problems:

  1. None of the changes seem problematic. They appear to be updates and improvements to the documentation. However, without the full context of the project and its requirements, it is difficult to assess if the changes fully meet the project's needs.

Commit a6deedf726e132730c1d3afadf6913da2abb0e90

Key changes:

  • The compile command wasmedgec has been replaced with wasmedge compile in two places: docs/develop/rust/socket_networking/client.md and docs/develop/rust/socket_networking/nonblock_client.md.

Potential problems:

  • It seems like the documentation update is focused on the replacement of the wasmedgec command with wasmedge compile. However, there is no explanation or justification for this change. It would be helpful to provide some context or reasoning behind this modification to aid understanding for users.
  • It is unclear if there are any compatibility issues or differences in functionality between the two commands. These details should be clarified in the documentation to ensure a smooth transition for users.
  • There are no other concerns or problems identified in this patch.

Commit 63618c27341b62f543693440842628907481ad8b

Key changes in the patch:

  • Changed the compile command for three different Rust code files (wasmedge_hyper_client.wasm, get.wasm, and wasmedge_reqwest_demo.wasm) in the client.md file.
  • Replaced wasmedgec with wasmedge compile for all compile commands.

Potential problems:

  • No potential problems were identified in this patch. The changes seem to be straightforward and consistent across the file.

Commit c35026d7dd0a48b606f6da02b47cba52e8ea4535

Key changes:

  • The patch updates the compile command from wasmedgec to wasmedge compile in two places in the server.md file.

Potential problems:

  • There don't appear to be any potential problems with this patch. The changes are straightforward and seem to be correct.

Overall, this patch updates the compile command for better performance in the documentation related to the Rust HTTP server examples.

Commit a6fb662cb89859e4f7edaae5cbc369e12d25508f

Key changes:

  • The networking.md file has been updated with changes to the content.
  • The order of the sections under the "Networking" heading has been changed.

Potential problems:

  • The summary of changes does not describe the specific modifications made to the content of networking.md. More details are needed to understand the extent of the changes and potential problems.

Commit 10598aa6c901d459fecfd1d178a18c4761d8c328

Key changes:

  • The wasmedgec command has been replaced with wasmedge compile.

Potential problems:

  • It's unclear why the command was changed from wasmedgec to wasmedge compile. The reason for this change should be explained in the pull request or in the documentation itself. Without this information, it's difficult to determine if the change is necessary or if it introduces any issues.
  • The patch only shows the change for the hello_world.md file. It's possible that this change also needs to be applied to other files in the repository. The pull request should clarify if this is the case.
  • The version of the wasmedge-quickjs release (v0.5.0-alpha) is hardcoded in the command. This could lead to issues if a different version is used in the future. It might be more flexible to use a variable or parameter to specify the version.
  • There is a typo in the patch description. The word "command" is missing an "d" at the end.

Suggestions:

  • Request clarification from the author of the pull request regarding the reason for changing the command and any potential issues that may arise from this change.
  • Ensure that the change is reflected in all relevant files, if applicable.
  • Consider using a variable or parameter to specify the version of wasmedge-quickjs instead of hardcoding it.

Commit f073b126eb4921dae31fb1dd4fb210b3c9746732

Key Changes:

  • The installation instructions have been updated to simplify and clarify the steps.
  • The links to the installation guides have been modified to point to the appropriate locations.

Potential Problems:

  • The patch appears to be focused on updating the installation instructions for WasmEdge and WasmEdge-QuickJS. However, there may be other changes or updates to the document that are not mentioned in the patch summary. A thorough review of the entire document is recommended to ensure that all relevant changes are captured and documented.
  • The patch does not provide any explanation or context for why these changes were made. It would be helpful to have a description or rationale for the modifications in the commit message or as a comment in the code.
  • The patch does not include any tests or verification to ensure that the updated installation instructions are accurate and complete. It would be beneficial to include steps to validate the instructions, such as a sample installation or verification process.

Overall, the changes in the patch seem to be focused on improving the clarity and accuracy of the installation instructions. However, further review and validation may be necessary to ensure the changes are correct and comprehensive.

Commit 3bd0c51f0d061be5905db9a3bf7764d9fc698ae9

Key changes:

  • Fixed broken links in the networking documentation.

Potential problems:

  • There don't appear to be any potential problems with this patch. It simply fixes broken links in the documentation.

Commit f1876dd3b646c98e0125d2be5a162ce6490bd718

Key changes:

  1. The download link for the WasmEdge TLS plugin has been updated to version 0.2.0.
  2. The command to unzip the downloaded file has been changed to use the unzip command instead of tar.
  3. The path to the compiled plugin library has been updated to target/release/libwasmedge_rustls.so.

Potential problems:

  1. The download link and file names may need to be verified to ensure they are accurate and up-to-date.
  2. The plugin installation instructions assume the user has already installed WasmEdge. It may be helpful to include instructions on how to install WasmEdge if it hasn't been installed yet.
  3. The commands to copy the plugin library to the appropriate directory assume a specific file structure and may not be applicable to all users. Additional instructions or checks may be necessary to handle different installation scenarios.
  4. The link at the end of the document references a chapter titled "HTTPS request in Rust", but it is unclear if this is the correct reference. It may need to be checked and updated if necessary.

Commit 12b57baf30021b7693bd22fab610756330f48577

Key changes in this patch:

  1. Improved HTTPS instructions in the docs/develop/rust/http_service/client.md file.
  2. Added a new code example for an HTTPS GET request in the docs/develop/rust/http_service/client.md file.
  3. Added a new code example for an HTTPS POST request in the docs/develop/rust/http_service/client.md file.
  4. Improved the instructions for the http_req_wasi crate in the docs/develop/rust/http_service/client.md file.
  5. Added a new code example for an HTTPS GET request using the http_req_wasi crate.
  6. Added a new code example for an HTTPS POST request using the http_req_wasi crate.

Potential problems:

  1. The patch does not mention why the HTTPS instructions were improved or if there were any issues with the previous instructions.
  2. The patch does not provide detailed explanations for the code examples or any relevant context.
  3. The patch assumes that readers are already familiar with certain concepts and technologies (e.g., WasmEdge, Rust, hyper crate, http_req_wasi crate).
  4. The patch does not address potential security concerns or best practices when dealing with HTTPS requests.
  5. The patch does not mention if any tests were added or updated alongside the code changes.

Overall, the patch mainly focuses on improving the instructions and adding new code examples for HTTPS requests. However, it lacks some important information and details that would enhance the clarity, context, and security of the documentation.

@juntao juntao changed the title Update client.md Networking related docs update Aug 19, 2023
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
Signed-off-by: Michael Yuan <[email protected]>
@juntao juntao marked this pull request as ready for review August 20, 2023 19:28
@juntao juntao requested a review from alabulei1 August 20, 2023 19:28
@alabulei1 alabulei1 merged commit 8a7b1bf into main Aug 21, 2023
6 checks passed
@alabulei1 alabulei1 deleted the juntao-networking-update branch August 21, 2023 02:51
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