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

ci: add mooncake for test-on-main #397

Merged
merged 1 commit into from
Oct 16, 2024
Merged

ci: add mooncake for test-on-main #397

merged 1 commit into from
Oct 16, 2024

Conversation

Young-Flash
Copy link
Collaborator

@Young-Flash Young-Flash merged commit 8bbb200 into main Oct 16, 2024
5 checks passed
@Young-Flash Young-Flash deleted the fix_test_on_main_ci branch October 16, 2024 06:46
Copy link

Based on the provided git diff output, here are three observations and suggestions:

  1. Missing Execution Permission for mooncake:

    • The diff shows that mooncake is being downloaded for both Linux and Windows, but the execution permission is only set for Linux (chmod +x ~/.moon/bin/moon*). Ensure that the execution permission is also set for Windows, although Windows does not use chmod. It might be worth adding a note or a separate command for setting the execution permission on Windows if such a concept exists in PowerShell.
  2. Potential Typos in URLs:

    • Ensure that the URLs used for downloading moonrun and mooncake are correct and accessible. There are no direct typos in the provided diff, but always double-check URLs in scripts to avoid runtime errors due to unreachable resources.
  3. Consistency in Directory Creation:

    • The script creates directories using mkdir -p ~/.moon/lib for Linux and New-Item -ItemType Directory -Force -Path "$env:USERPROFILE/.moon/lib" for Windows. While both methods are correct, ensure that the paths and methods used across different operating systems are consistent to reduce confusion and potential errors.

These suggestions are based on the provided diff and aim to improve the robustness and consistency of the script across different operating systems.

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.

1 participant