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

docs(packages): Update README.md #178

Merged
merged 8 commits into from
Dec 20, 2023

Conversation

wuhuizuo
Copy link
Contributor

also add the the verify step for the README.md with shelldoc tool

Ref: https://github.com/endocode/shelldoc
Signed-off-by: wuhuizuo wuhuizuo@126.com

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind December 19, 2023 12:41
Copy link

ti-chi-bot bot commented Dec 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from wuhuizuo. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

ti-chi-bot bot commented Dec 19, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
This pull request updates the README.md file and adds a verification step using the shelldoc tool. The changes look good and do not seem to introduce any potential issues. However, there are some suggestions to improve the implementation:

  • The installation of both gomplate and shelldoc can be combined into a single run command to reduce the number of steps.
  • The yq and jq tools should be installed before being used in the GitHub actions workflow.
  • In the get-package-builder-with-config.sh script, the yq command should include the -e flag to exit with a non-zero status code if the query returns no results.

Overall, the changes are minor and the implementation looks good. Therefore, this pull request can be merged.

@ti-chi-bot ti-chi-bot bot added the size/M label Dec 19, 2023
@wuhuizuo wuhuizuo force-pushed the feature/add-builder-for-tidb-operator branch from 21fe1de to 4384ad4 Compare December 19, 2023 12:45
Copy link

ti-chi-bot bot commented Dec 19, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
This pull request updates the README.md file and adds a verification step for it using the shelldoc tool. It also updates the pull-verify-packages-config.yaml file to include the installation of shelldoc and the verification step for the README.md file.

The potential problem with this pull request is that it updates the pull-verify-packages-config.yaml file to install shelldoc and verify the README.md file, but it does not provide any instructions on how to install shelldoc. It assumes that the tool is already installed, but this may not be the case for everyone.

A possible fix for this would be to add instructions on how to install shelldoc in the README.md file or in the pull request description. Another fix would be to include the installation of shelldoc in the pull-verify-packages-config.yaml file, so that it is installed automatically when the verification step is run.

Other than that, the changes seem to be straightforward and do not introduce any significant issues.

also add the the verify step for the README.md with shelldoc tool

Ref: https://github.com/endocode/shelldoc
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/add-builder-for-tidb-operator branch from 4384ad4 to 802c368 Compare December 19, 2023 12:51
Copy link

ti-chi-bot bot commented Dec 19, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
The pull request updates the README.md file in the packages directory and adds a verification step for it using the shelldoc tool. Additionally, it updates the pull-verify-packages-config.yaml file to install shelldoc and configure it to verify the README.md file. The changes to the offline-packages.yaml.tmpl, packages.yaml.tmpl, and get-package-builder-with-config.sh files are to add a link to the updated README.md file.

There are no potential problems with the changes in this pull request. However, it would be beneficial to add a comment that explains what the shelldoc tool is and why it is being used in the verification step. This would help other contributors understand the purpose of the changes and the importance of the verification step.

For fixing suggestions, the pull request is fine as is. However, if the suggested comment is added, it should be clear and concise, explaining the purpose of the shelldoc tool in the verification step.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Dec 19, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
This pull request updates the README.md file and adds a verification step for the README.md using the shelldoc tool. The changes also include some modifications to the .github/workflows/pull-verify-packages-config.yaml file, adding the installation of the shelldoc tool and commenting out a verification step. The changes to the README.md file add some information about the tools required for the project and their installation, as well as some required context for offline deploy packages.

There do not seem to be any potential problems with this pull request. However, the changes to the .github/workflows/pull-verify-packages-config.yaml file could be modified to use a single run command for installing both the gomplate and shelldoc tools. Additionally, the commented-out verification step in the same file could be removed entirely if it is no longer needed.

Here are some possible changes to the .github/workflows/pull-verify-packages-config.yaml file:

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - name: Install tools
        run: |
-          git clone https://github.com/hairyhenderson/gomplate
-          cd gomplate
-          GOBIN=/usr/local/bin go install ./cmd/gomplate
-          rm -rf gomplate
-          gomplate --version
+          # install gomplate and shelldoc
+          git clone https://github.com/hairyhenderson/gomplate && \
+            pushd gomplate && \
+            GOBIN=/usr/local/bin go install ./cmd/gomplate &&
+            popd &&
+            rm -rf gomplate && \
+            gomplate --version
+          git clone https://github.com/endocode/shelldoc && \
+            pushd shelldoc && \
+            GOBIN=/usr/local/bin go install ./cmd/shelldoc &&
+            popd &&
+            rm -rf shelldoc && \
+            shelldoc --version

      - name: Checkout code
        uses: actions/checkout@v4

      # - name: Verify the README.md
      #   run: |
      #     shelldoc run --shell `which bash` packages/README.md

And here is the modified version of the same file with the commented-out verification step removed:

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - name: Install tools
        run: |
-          git clone https://github.com/hairyhenderson/gomplate
-          cd gomplate
-          GOBIN=/usr/local/bin go install ./cmd/gomplate
-          rm -rf gomplate
-          gomplate --version
+          # install gomplate and shelldoc
+          git clone https://github.com/hairyhenderson/gomplate && \
+            pushd gomplate && \
+            GOBIN=/usr/local/bin go install ./cmd/gomplate &&
+            popd &&
+            rm -rf gomplate && \
+            gomplate --version
+          git clone https://github.com/endocode/shelldoc && \
+            pushd shelldoc && \
+            GOBIN=/usr/local/bin go install ./cmd/shelldoc &&
+            popd &&
+            rm -rf shelldoc && \
+            shelldoc --version

      - name: Checkout code
        uses: actions/checkout@v4

@ti-chi-bot ti-chi-bot bot added size/L and removed size/M labels Dec 19, 2023
Copy link

ti-chi-bot bot commented Dec 19, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
The pull request updates the README.md and adds a verification step for it using the shelldoc tool. The PR also installs the shelldoc tool and yq as a prerequisite for the verification step.

There are no potential problems in the changes, but the pull request description is not clear enough to understand the purpose of the changes.

One suggestion would be to add a brief summary of the changes in the PR description. Additionally, it might be useful to add some context on why the shelldoc tool is being used and how it helps with the project.

Overall, this is a simple and straightforward change that adds a useful verification step to ensure the README.md is up-to-date.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/add-builder-for-tidb-operator branch from 4ddfd83 to 5eb2d29 Compare December 19, 2023 13:22
Copy link

ti-chi-bot bot commented Dec 19, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
Pull Request Review

The pull request updates the README.md file and adds a step for the verification process using the shelldoc tool. The changes appear to be straightforward and do not introduce any significant problems. However, some potential issues and suggestions are:

  1. The shelldoc tool should be added to the prerequire tools section of the README.md file.

  2. The commented out code for the Verify the README.md step should be removed.

  3. The get-package-builder-with-config.sh script should be updated to handle a case where the yq command returns an empty result. It should print an error message and exit with a non-zero code instead of continuing to execute.

  4. The go command for installing shelldoc and gomplate should be moved to a separate line for better readability.

  5. The README.md file should be updated to reflect the changes and provide clear instructions for using shelldoc.

  6. The yq and jq tools should be added to the prerequire tools section of the README.md file.

  7. The offline-packages.yaml.tmpl file should be updated to reflect the changes made to the README.md file and provide clear instructions for using yq.

Overall, the changes appear to be reasonable, and I would recommend merging the pull request once the suggested improvements are made.

Copy link

ti-chi-bot bot commented Dec 20, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Summary

This pull request updates the README.md file in the packages directory and adds a verification step for it using the shelldoc tool. The changes include adding the required tools for the verification as well.

Potential Problems

  1. The verification step for README.md is commented out, so it's unclear if it is working or not.
  2. The get-package-builder-with-config.sh script exits with code 0 when no package routes are matched, but it should exit with a non-zero code to indicate an error.

Fix Suggestions

  1. Uncomment the verification step for README.md and run it to ensure it works as expected.
  2. Change the exit 0 statement in get-package-builder-with-config.sh to exit 1 to indicate an error when no package routes are matched.

packages/packages.yaml.tmpl Outdated Show resolved Hide resolved
Copy link

ti-chi-bot bot commented Dec 20, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Review for docs(packages): Update README.md

Summary

This pull request updates the README.md file in the packages directory. The changes include adding a verification step to the README.md using shelldoc tool and updating the document to include the tools required to work with the package configurations.

Potential problems

  • The pull request does not add any value to the codebase of the project itself, as it is a documentation change.
  • The push operation does not trigger any unit tests or validation checks, making it difficult to know whether the changes are valid or not.
  • The use of pushd and popd in the script may cause issues in some shells.

Suggestions

  • It is recommended to include a documentation check in the CI/CD pipeline, so any changes to the documentation are validated before merging them into the codebase.
  • The use of pushd and popd could be replaced with a simple cd command to avoid shell issues.

Overall

The changes in this pull request are minor and do not add any value to the codebase. However, the added verification step and updating the document to include required tools are useful changes.

packages/packages.yaml.tmpl Outdated Show resolved Hide resolved
packages/packages.yaml.tmpl Outdated Show resolved Hide resolved
Copy link

ti-chi-bot bot commented Dec 20, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Review for docs(packages): Update README.md

Summary

This pull request is updating the README.md file for the package. The changes made to the file include adding a verify step using the shelldoc tool, updating the tools needed for the package, and adding more detailed information about the package.

Potential Problems

  • There are no potential problems identified in this pull request.

Suggestions

  • No suggestions are needed for this pull request.

Overall, the changes made in this pull request seem to be appropriate and properly documented. No issues or potential problems were identified. Therefore, this pull request can be merged.

Copy link

ti-chi-bot bot commented Dec 20, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.

Pull Request Review

Summary

This pull request updates the README.md file and adds a verification step using the shelldoc tool.

Changes Made

  • Added shelldoc tool to the workflow for verifying the README.md file.
  • Updated the packages README.md file
  • Fixed some formatting issues

Potential Problems

  • The shelldoc tool is not a widely used tool, so it may cause issues for people who are not familiar with it.
  • The description of the pull request is not very clear, it would be better to explain the changes in more detail.

Suggestions

  • Add a brief explanation of what shelldoc tool is and how it works to the README.md file.
  • Provide an example of how to use the shelldoc tool in the README.md file.
  • Provide a more detailed explanation of the changes made in the pull request description.
  • Consider using a more widely used tool for verification, such as shellcheck.

@wuhuizuo wuhuizuo merged commit 7f218cb into main Dec 20, 2023
1 check failed
@wuhuizuo wuhuizuo deleted the feature/add-builder-for-tidb-operator branch December 20, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant