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

fix(scripts/artifacts): fix check-images-internal.sh #3046

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

wuhuizuo
Copy link
Collaborator

@wuhuizuo wuhuizuo commented Aug 2, 2024

Also add Deno scripts:

  • scripts/artifacts/check-image-internal.ts
  • scripts/artifacts/check-tiup.sh
  • ops/search-lark-user-by-githubid.ts

Signed-off-by: wuhuizuo wuhuizuo@126.com

Also add Deno scripts:
- scripts/artifacts/check-image-internal.ts
- scripts/artifacts/check-tiup.sh
- ops/search-lark-user-by-github.ts

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind August 2, 2024 03:08
@ti-chi-bot ti-chi-bot bot added the size/XXL label Aug 2, 2024
Copy link

ti-chi-bot bot commented Aug 2, 2024

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

This pull request mainly includes changes in two aspects:

  1. The PR fixes the script scripts/artifacts/check-images-internal.sh. Specifically, it removes the lines where publish_time is reassigned with the value extracted from $platforms. This is probably because publish_time is already correctly assigned with the created time of the image from tmp-image-config.yaml, and reassigning it may cause incorrect time to be recorded.

  2. The PR adds three new Deno scripts:

    • scripts/artifacts/check-image-internal.ts: This script checks the image info for different platforms and validates if the git sha and published time are consistent across different platforms. It also generates a yaml file to record the check results.
    • scripts/artifacts/check-tiup.ts: This script checks the package info from Tiup and compares it with the info from OCI. It validates if the git sha, published time, and available platforms are consistent. It also generates a yaml file to record the check results.
    • ops/search-lark-user-by-github.ts: This script is used to search for a user on Lark by their GitHub ID.

Potential issues and suggestions:

  1. There is no clear indication that any testing has been done for these scripts. It would be ideal to include some test cases to ensure the scripts are working as expected.

  2. It seems like the search-lark-user-by-github.ts script throws an error when the user search operation fails. However, it might be more user-friendly to just print an error message and exit the script gracefully, instead of throwing an error that could potentially crash the script.

  3. For both check-image-internal.ts and check-tiup.ts, an error message is printed if something goes wrong during the check, but the script continues to execute. It might be better to add some error handling logic so that the script can stop and exit with an error code when it encounters an error.

  4. In check-image-internal.ts and check-tiup.ts, the variables ociInfos and results are declared as const, but they are later mutated. While this is allowed in JavaScript and TypeScript, it can be confusing. It might be more clear to declare these variables with let instead.

  5. In check-image-internal.ts and check-tiup.ts, the Deno permissions --allow-run, --allow-net, and --allow-write are used. It's good practice to only grant the minimum permissions that a script requires. If any of these permissions are not actually needed, they should be removed.

  6. The scripts make use of external dependencies (like "jsr:@std/yaml@^1.0.0" and "jsr:@std/cli@^1.0.1"). It's important to ensure these dependencies are trusted and kept up-to-date to avoid potential security issues.

  7. The scripts are missing error handling for network requests (like the fetch() call in gatheringGithubGitSha function and gatherOciMetadata function). It would be good to add error handling here to manage network errors or failures.

@wuhuizuo
Copy link
Collaborator Author

wuhuizuo commented Aug 2, 2024

/approve

Copy link

ti-chi-bot bot commented Aug 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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

The pull request process is described 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

@ti-chi-bot ti-chi-bot bot added the approved label Aug 2, 2024
@wuhuizuo wuhuizuo added the lgtm label Aug 2, 2024
@ti-chi-bot ti-chi-bot bot merged commit e86640f into main Aug 2, 2024
1 check passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-easy-use-check-scripts branch August 2, 2024 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant