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

chore: update release action #2459

Closed
wants to merge 6 commits into from
Closed

Conversation

MrKou47
Copy link
Member

@MrKou47 MrKou47 commented Dec 12, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Other information:

Summary by CodeRabbit

  • New Features

    • Enhanced release process with new steps for checking out repositories, caching, and building.
    • Introduction of a mock package.json for the platform adapter.
    • Added steps for releasing engine packages and syncing with the CDN.
  • Improvements

    • Renamed steps for clarity and specificity in the workflow.
    • Explicit installation of pnpm and creation of a distribution directory.
    • Added a caching step for the Engine package directory based on the runner's OS and commit SHA.
    • New dependency added: @actions/core in devDependencies.

Copy link

coderabbitai bot commented Dec 12, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The changes in the pull request primarily focus on enhancing the release workflow defined in .github/workflows/release.yml. Key modifications include the introduction of new steps, renaming existing steps for clarity, and implementing caching for the Engine package. The workflow now includes steps to check out sub-repositories, establish a distribution directory, create a mock package.json, and configure environment variables for bundling. Additionally, the workflow facilitates the release of engine packages and synchronization with the CDN using the galacean/publish action.

Changes

File Change Summary
.github/workflows/release.yml - Added "Checkout Engine repo" step.
- Renamed "Build" to "Build Engine."
- Introduced caching step for Engine package.
- Added steps to check out sub-repositories for engine toolkit, Lottie, and Spine.
- Ensured distribution directory exists and created mock package.json in platform-adapter.
- Modified environment variables for "Bundle polyfill and engine" step.
- Included steps for releasing engine packages and syncing platform adapter to CDN.
package.json - Added new dependency @actions/core with version ^1.11.1 in devDependencies.

Poem

In the meadow where code does play,
A rabbit hops with joy today.
New steps added, clear and bright,
The workflow shines, a true delight!
With caching and tools all in tow,
Off to the release, let the packages flow! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a50e6c0 and 12fd732.

📒 Files selected for processing (1)
  • .github/workflows/release.yml (2 hunks)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@MrKou47 MrKou47 requested a review from GuoLei1990 December 12, 2024 15:25
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
.github/workflows/release.yml (3)

40-44: Optimize cache configuration

Consider adding a fallback cache key to improve cache hit rates. This allows using a previous cache when the exact commit SHA doesn't match.

  uses: actions/cache@v3
  with:
    path: ./packages/galacean
-   key: ${{ runner.os }}-engine-${{ github.sha }}
+   key: ${{ runner.os }}-engine-${{ github.sha }}
+   restore-keys: |
+     ${{ runner.os }}-engine-

80-82: Add error handling for version extraction

The version extraction process should handle potential errors and validate the version format.

  run: |
-   VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
+   VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
+   if [[ ! $VERSION =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?(\+[0-9A-Za-z-]+)?$ ]]; then
+     echo "Error: Invalid version format: $VERSION"
+     exit 1
+   fi
    echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json

Node.js version requirement should be lowered to LTS

The specified Node.js version requirement of '>=22.6.0' is unnecessarily high. Based on the codebase analysis:

  • No package.json files specify any Node.js engine requirements
  • The codebase primarily consists of TypeScript/JavaScript files using standard ES modules syntax
  • The build tools used (rollup, typescript, etc.) are all compatible with Node.js LTS versions

Recommend changing to Node.js 20.x (Current LTS) as it provides all necessary features while being production-ready.

🔗 Analysis chain

Line range hint 31-33: Verify Node.js version requirement

The specified Node.js version requirement of '>=22.6.0' seems unusually high. Node.js 22 is currently in pre-release status and not recommended for production use. Consider using the current LTS version (20.x) instead.

- node-version: '>=22.6.0'
+ node-version: '20.x'
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if any dependencies require Node.js 22
fd package.json --exec jq -r '.engines.node' {} 2>/dev/null

Length of output: 124


Script:

#!/bin/bash
# Check for any Node.js version requirements in package.json files
fd package.json --exec cat {} | grep -A 5 '"engines"' 2>/dev/null

# Also check for any Node.js-specific features or syntax in JavaScript/TypeScript files
rg -l --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx,mjs}' --type ts,js '(?:import\s+.*\s+from|export\s+.*\s+from|import\s*\(|import\.meta)' 

Length of output: 235


Script:

#!/bin/bash
# Check for any Node.js version requirements in package.json files
fd package.json --exec cat {}

# Check for modern JavaScript features in source files
rg -t js -t ts "(?:import\s+.*\s+from|export\s+.*\s+from|import\s*\(|import\.meta)" -l

Length of output: 47316

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ae6eab and 6a4dc84.

📒 Files selected for processing (1)
  • .github/workflows/release.yml (2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yml

83-83: could not parse as YAML: yaml: line 83: did not find expected key

(syntax-check)

🪛 yamllint (1.35.1)
.github/workflows/release.yml

[error] 106-106: syntax error: expected , but found '-'

(syntax)

.github/workflows/release.yml Outdated Show resolved Hide resolved
Comment on lines 46 to 72
- name: Checkout Sub-repositories
run: |
git clone https://github.com/galacean/engine-toolkit.git
git clone https://github.com/galacean/engine-lottie.git
git clone https://github.com/galacean/engine-spine.git

- name: Install and Link Engine for Toolkit
run: |
cd toolkit
pnpm install
pnpm link ../packages/galacean
pnpm b:all

- name: Install and Link Engine for Lottie
run: |
cd toolkit
pnpm install
pnpm link ../packages/galacean
pnpm build

- name: Install and Link Engine for Spine
run: |
cd toolkit
pnpm install
pnpm link ../packages/galacean
pnpm build

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor repeated setup logic

The setup logic for sub-repositories is duplicated. Consider creating a reusable composite action or shell script.

Example implementation:

- name: Setup Sub-repository
  run: |
    setup_repo() {
      local repo_name=$1
      cd $repo_name
      pnpm install
      pnpm link ../packages/galacean
      pnpm ${2:-build}
      cd ..
    }
    
    setup_repo "engine-toolkit" "b:all"
    setup_repo "engine-lottie"
    setup_repo "engine-spine"

.github/workflows/release.yml Outdated Show resolved Hide resolved
Comment on lines 84 to 85
- name: Bundle polyfill and engine
uses: galacean/platform-adapter@tag
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix invalid action reference

The action reference uses: galacean/platform-adapter@tag appears to use a literal "tag" instead of a specific version or ref.

- uses: galacean/platform-adapter@tag
+ uses: galacean/platform-adapter@v1  # Replace with actual version

Committable suggestion skipped: line range outside the PR's diff.

.github/workflows/release.yml Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.48%. Comparing base (0ae6eab) to head (12fd732).
Report is 21 commits behind head on dev/1.4.

Additional details and impacted files
@@             Coverage Diff             @@
##           dev/1.4    #2459      +/-   ##
===========================================
+ Coverage    68.33%   68.48%   +0.14%     
===========================================
  Files          908      921      +13     
  Lines        94377    95425    +1048     
  Branches      7926     8119     +193     
===========================================
+ Hits         64492    65348     +856     
- Misses       29635    29825     +190     
- Partials       250      252       +2     
Flag Coverage Δ
unittests 68.48% <ø> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- name: Install and Link Engine for Lottie
run: |
cd toolkit
pnpm install
Copy link
Member

Choose a reason for hiding this comment

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

Here is a problem: the version numbers of these packages do not match the engine

.github/workflows/release.yml Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🔭 Outside diff range comments (1)
.github/workflows/release.yml (1)

Line range hint 31-33: Specify exact Node.js version instead of using a range

Using >=22.6.0 could lead to unexpected behavior if a new Node.js version introduces breaking changes. Consider using a specific LTS version.

       with:
-          node-version: '>=22.6.0'
+          node-version: '20.10.0'  # Latest LTS version
           registry-url: https://registry.npmjs.org/
🧰 Tools
🪛 actionlint (1.7.4)

19-19: step must run script with "run" section or run action with "uses" section

(syntax-check)

♻️ Duplicate comments (1)
.github/workflows/release.yml (1)

54-54: ⚠️ Potential issue

Fix incorrect directory paths

The cd commands are using wrong directory names.

-          cd toolkit
+          cd engine-toolkit

-          cd lottie
+          cd engine-lottie

-          cd spine
+          cd engine-spine

Also applies to: 61-61, 68-68

🧹 Nitpick comments (2)
.github/workflows/release.yml (2)

40-44: Optimize cache configuration

The current cache configuration could be improved:

  1. Include package.json hash in the cache key for more precise caching
  2. Add restore-keys for fallback cache hits
       with:
         path: ./packages/galacean
-          key: ${{ runner.os }}-engine-${{ github.sha }}
+          key: ${{ runner.os }}-engine-${{ hashFiles('**/package.json') }}-${{ github.sha }}
+          restore-keys: |
+            ${{ runner.os }}-engine-${{ hashFiles('**/package.json') }}-
+            ${{ runner.os }}-engine-
🧰 Tools
🪛 actionlint (1.7.4)

41-41: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


106-127: Optimize environment variables configuration

Environment variables are duplicated between steps. Move common variables to the job level.

jobs:
  release:
+    env:
+      NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
+      NPM_CONFIG_PROVENANCE: true
+      OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
+      OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
+      OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a4dc84 and f47a9c9.

📒 Files selected for processing (1)
  • .github/workflows/release.yml (2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yml

19-19: step must run script with "run" section or run action with "uses" section

(syntax-check)


41-41: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🔇 Additional comments (1)
.github/workflows/release.yml (1)

52-71: Refactor repeated sub-repository setup logic

The setup logic is duplicated across three sub-repositories. Consider creating a reusable function.

+      - name: Setup Sub-repositories
+        run: |
+          setup_repo() {
+            local repo_name=$1
+            cd $repo_name
+            pnpm install
+            pnpm link ../packages/galacean
+            pnpm ${2:-build}
+            cd ..
+          }
+          
+          setup_repo "engine-toolkit" "b:all"
+          setup_repo "engine-lottie"
+          setup_repo "engine-spine"

steps:
- name: Checkout Engine repo
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix YAML syntax error in checkout step

The step definition has incorrect syntax. The uses key should be under the name key.

-      - name: Checkout Engine repo
-      - uses: actions/checkout@v4
+      - name: Checkout Engine repo
+        uses: actions/checkout@v4
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Checkout Engine repo
- name: Checkout Engine repo
uses: actions/checkout@v4
🧰 Tools
🪛 actionlint (1.7.4)

19-19: step must run script with "run" section or run action with "uses" section

(syntax-check)

echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json

- name: Bundle polyfill and engine
uses: galacean/platform-adapter@main
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use specific version for platform-adapter action

Using the main branch is risky as it can change unexpectedly. Pin to a specific version or commit SHA.

-        uses: galacean/platform-adapter@main
+        uses: galacean/[email protected]  # Replace with actual version

Committable suggestion skipped: line range outside the PR's diff.

"outputDir": "${{ github.workspace }}/platform-adapter/dist"
}

- name: Release engine packages and Sync to CDN
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Update the galacean/publish action to v0.2.2

The current workflow uses galacean/[email protected], but a newer version v0.2.2 is available. Consider updating the action version to use the latest stable release.

🔗 Analysis chain

Verify galacean/publish action version

The action galacean/[email protected] is used in both release steps. Please verify if this is the latest stable version.

Also applies to: 115-115


Let me try another approach to verify the action version.


Let me try one more approach to check the tags directly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check latest version and release history of galacean/publish action

# Check latest release
gh api repos/galacean/publish/releases/latest --jq '.tag_name'

# List recent releases
gh api repos/galacean/publish/releases --jq '.[].tag_name'

Length of output: 278


Script:

#!/bin/bash
# Check the repository and its contents directly
gh api repos/galacean/publish/contents/.github/workflows --jq '.[].name'

# Try to get the repository information
gh api repos/galacean/publish

Length of output: 16042


Script:

#!/bin/bash
# Check tags in the repository
gh api repos/galacean/publish/git/refs/tags --jq '.[].ref'

# Try to check the action.yml file in the repository
gh api repos/galacean/publish/contents/action.yml --jq '.content' | base64 -d

Length of output: 299

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
.github/workflows/release.yml (1)

Line range hint 31-33: Fix Node.js version constraint

Using >=22.6.0 for Node.js version is risky as it may lead to unexpected behavior with future Node.js releases. Instead, specify an exact version or a more constrained range.

       uses: actions/setup-node@v4
       with:
-         node-version: '>=22.6.0'
+         node-version: '22.x'
          registry-url: https://registry.npmjs.org/
          cache: pnpm
🧰 Tools
🪛 actionlint (1.7.4)

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

♻️ Duplicate comments (3)
.github/workflows/release.yml (3)

51-71: ⚠️ Potential issue

Fix incorrect directory paths and reduce duplication

  1. The cd commands use incorrect directory names that don't match the cloned repositories.
  2. The installation steps are duplicated across sub-repositories.
       - name: Install and Link Engine for Toolkit
         run: |
-          cd toolkit
+          cd engine-toolkit
           pnpm install
           pnpm link ../packages/galacean
           pnpm b:all

       - name: Install and Link Engine for Lottie
         run: |
-          cd lottie
+          cd engine-lottie
           pnpm install
           pnpm link ../packages/galacean
           pnpm build

       - name: Install and Link Engine for Spine
         run: |
-          cd spine
+          cd engine-spine
           pnpm install
           pnpm link ../packages/galacean
           pnpm build

Consider refactoring the repeated setup logic into a reusable shell function:

      - name: Install and Link Sub-repositories
        run: |
          setup_repo() {
            local repo_name=$1
            local build_cmd=${2:-build}
            cd $repo_name
            pnpm install
            pnpm link ../packages/galacean
            pnpm $build_cmd
            cd ..
          }
          
          setup_repo "engine-toolkit" "b:all"
          setup_repo "engine-lottie"
          setup_repo "engine-spine"

83-104: ⚠️ Potential issue

Fix action version and add path validation

  1. Using @main for the platform-adapter action is risky as it can change unexpectedly.
  2. The bundle paths should be validated before use.
-      uses: galacean/platform-adapter@main
+      uses: galacean/[email protected]  # Replace with actual version
       env:
         ADAPTER_BUNDLE_SETTINGS: |
           {
             "polyfill": true,
             "engine": [
               "${{ github.workspace }}/packages/galacean/dist/module.js",
               "${{ github.workspace }}/packages/xr/dist/module.js",
               "${{ github.workspace }}/packages/shader-lab/dist/module.js",
               "${{ github.workspace }}/packages/physics-lite/dist/module.js",
               "${{ github.workspace }}/packages/physics-physx/dist/module.js",
               "${{ github.workspace }}/engine-lottie/dist/module.js",
               "${{ github.workspace }}/engine-spine/dist/module.js",
               "${{ github.workspace }}/engine-toolkit/galacean-engine-toolkit/dist/module.js"
             ],
             "jsWASMLoader": [
               "${{ github.workspace }}/packages/physics-physx/libs/physx.release.js"
             ],
             "outputDir": "${{ github.workspace }}/platform-adapter/dist"
           }

Add a validation step before bundling:

      - name: Validate Bundle Paths
        run: |
          validate_path() {
            if [ ! -f "$1" ]; then
              echo "Error: File not found: $1"
              exit 1
            fi
          }
          
          validate_path "${{ github.workspace }}/packages/galacean/dist/module.js"
          validate_path "${{ github.workspace }}/packages/xr/dist/module.js"
          # Add validation for other paths

105-126: 🛠️ Refactor suggestion

Optimize environment configuration and update action version

  1. Environment variables are duplicated between steps
  2. The action version should be updated to v0.2.2

Move common environment variables to job level:

jobs:
  release:
    env:
      NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
      NPM_CONFIG_PROVENANCE: true
      OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
      OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
      OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}

Update the action version:

-      uses: galacean/[email protected]
+      uses: galacean/[email protected]
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)

39-44: Enhance cache configuration

The cache configuration could be improved:

  1. Add restore-keys for fallback cache hits
  2. Include pnpm's lockfile hash in the cache key for more precise caching
       uses: actions/cache@v3
       with:
         path: ./packages/galacean
-        key: ${{ runner.os }}-engine-${{ github.sha }}
+        key: ${{ runner.os }}-engine-${{ hashFiles('**/pnpm-lock.yaml') }}-${{ github.sha }}
+        restore-keys: |
+          ${{ runner.os }}-engine-${{ hashFiles('**/pnpm-lock.yaml') }}-
+          ${{ runner.os }}-engine-
🧰 Tools
🪛 actionlint (1.7.4)

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)


79-81: Add error handling for version extraction

The version extraction could fail silently if the package.json is malformed or missing.

       run: |
+          if [ ! -f "${{ github.workspace }}/packages/galacean/package.json" ]; then
+            echo "Error: package.json not found"
+            exit 1
+          fi
           VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
+          if [ -z "$VERSION" ]; then
+            echo "Error: Failed to extract version"
+            exit 1
+          fi
           echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f47a9c9 and 521ad63.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • .github/workflows/release.yml (2 hunks)
  • package.json (1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yml

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🔇 Additional comments (1)
package.json (1)

27-27: LGTM: Addition of @actions/core dependency

The addition of @actions/core is appropriate for enhancing GitHub Actions workflow functionality.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
.github/workflows/release.yml (1)

86-103: Add path validation for bundle settings

The bundle configuration assumes all specified paths exist. Consider adding validation to ensure required files are present before bundling.

- name: Validate bundle paths
  run: |
    for path in \
      "${{ github.workspace }}/packages/galacean/dist/module.js" \
      "${{ github.workspace }}/packages/xr/dist/module.js" \
      # ... add other paths
    do
      if [ ! -f "$path" ]; then
        echo "Error: Required file not found: $path"
        exit 1
      fi
    done
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 521ad63 and bdc2c2c.

📒 Files selected for processing (1)
  • .github/workflows/release.yml (2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yml

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🔇 Additional comments (4)
.github/workflows/release.yml (4)

51-71: Refactor repeated setup logic

The setup logic for sub-repositories is duplicated.


84-84: Use specific version for platform-adapter action

Using the main branch is risky as it can change unexpectedly.


105-126: Optimize environment variable configuration

The environment variables are duplicated between the two publish steps. Consider moving them to the job level.

jobs:
  release:
    runs-on: ubuntu-latest
    permissions:
      contents: write
      id-token: write
+   env:
+     NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
+     NPM_CONFIG_PROVENANCE: true
+     OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
+     OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
+     OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}

Additionally, consider updating to the latest version of the publish action.


Line range hint 31-34: Verify Node.js version requirement

The specified Node.js version requirement >=22.6.0 seems unusually high, as Node.js 22 is still in development. This might cause compatibility issues.

Consider using an LTS version like 20.x:

- node-version: '>=22.6.0'
+ node-version: '20.x'
🧰 Tools
🪛 actionlint (1.7.4)

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

Comment on lines +79 to +81
run: |
VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for version extraction

The version extraction using jq lacks error handling. If the package.json is malformed or the version field is missing, this could fail silently.

- VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
+ VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
+ if [ -z "$VERSION" ]; then
+   echo "Error: Failed to extract version from package.json"
+   exit 1
+ fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
run: |
VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json
run: |
VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
if [ -z "$VERSION" ]; then
echo "Error: Failed to extract version from package.json"
exit 1
fi
echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json

Comment on lines +45 to +49
- name: Checkout Sub-repositories
run: |
git clone https://github.com/galacean/engine-toolkit.git
git clone https://github.com/galacean/engine-lottie.git
git clone https://github.com/galacean/engine-spine.git
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add authentication for git operations

Using unauthenticated git clone might hit GitHub's rate limits. Consider using a token for authentication:

- git clone https://github.com/galacean/engine-toolkit.git
+ git clone https://${{ secrets.GITHUB_TOKEN }}@github.com/galacean/engine-toolkit.git

Committable suggestion skipped: line range outside the PR's diff.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🔭 Outside diff range comments (1)
.github/workflows/release.yml (1)

Node.js version 22.6.0+ is too aggressive for production releases

The specified Node.js version requirement (>=22.6.0) in the release workflow is concerning because:

  • None of the package.json files specify any Node.js engine requirements
  • Node.js 22.x is currently in preview/beta status and not recommended for production use
  • The latest LTS version is Node.js 20.x, which would be more appropriate for a release workflow

Recommendation: Use Node.js 20.x LTS version (e.g., 20.x) in the release workflow for better stability and compatibility.

🔗 Analysis chain

Line range hint 31-33: Verify Node.js version compatibility

The specified Node.js version requirement (>=22.6.0) is quite high and may be in preview/beta status. Consider using an LTS version for better stability in the release workflow.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if any package.json files specify Node.js engine requirements
fd -e json -x jq -r '.engines.node // empty' {}

Length of output: 110


Script:

#!/bin/bash
# Check package.json content directly
fd "package.json" --type f --exec cat {}

Length of output: 12210

🧰 Tools
🪛 actionlint (1.7.4)

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

♻️ Duplicate comments (1)
.github/workflows/release.yml (1)

76-82: 🛠️ Refactor suggestion

Add error handling for version extraction

The version extraction lacks error handling and validation.

       - name: Create package.json in platform-adapter
         run: |
+          if [ ! -f "${{ github.workspace }}/packages/galacean/package.json" ]; then
+            echo "Error: package.json not found"
+            exit 1
+          fi
           VERSION=$(jq -r '.version' ${{ github.workspace }}/packages/galacean/package.json)
+          if [ -z "$VERSION" ] || [ "$VERSION" = "null" ]; then
+            echo "Error: Failed to extract version from package.json"
+            exit 1
+          fi
           echo "{\"name\": \"@galacean/engine-platform-adapter-release\", \"version\": \"$VERSION\"}" > ${{ github.workspace }}/platform-adapter/package.json
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)

106-127: Optimize workflow configuration

  1. Move common environment variables to job level
  2. Update galacean/publish action to latest version
   release:
     runs-on: ubuntu-latest
     permissions:
       contents: write
       id-token: write
+    env:
+      NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
+      NPM_CONFIG_PROVENANCE: true
+      OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
+      OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
+      OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}
     steps:
       # ... other steps ...
       - name: Release engine packages and Sync to CDN
-        uses: galacean/[email protected]
-        env:
-          NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
-          NPM_CONFIG_PROVENANCE: true
-          OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
-          OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
-          OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}
+        uses: galacean/[email protected]

       - name: Sync Platform Adapter to CDN
-        uses: galacean/[email protected]
+        uses: galacean/[email protected]
         with:
           publish: false
           packages: |
             platform-adapter
-        env:
-          NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
-          NPM_CONFIG_PROVENANCE: true
-          OASISBE_UPLOAD_URL: https://oasisbe.alipay.com/api/file/no-auth/crypto/upload
-          OASISBE_REQUEST_HEADER: ${{secrets.OASISBE_REQUEST_HEADER}}
-          OASISBE_PUBLIC_KEY: ${{secrets.OASISBE_PUBLIC_KEY}}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bdc2c2c and a50e6c0.

📒 Files selected for processing (1)
  • .github/workflows/release.yml (2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/release.yml

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🔇 Additional comments (3)
.github/workflows/release.yml (3)

18-19: LGTM: Proper repository checkout configuration

The fetch-depth: 0 setting is correct for release workflows as it ensures the full Git history is available.


51-71: Refactor repeated setup logic

The setup logic for sub-repositories is duplicated. Consider creating a reusable function.

+      - name: Setup Sub-repositories
+        run: |
+          setup_repo() {
+            local repo_name=$1
+            local build_cmd=${2:-build}
+            cd "$repo_name"
+            echo "Setting up $repo_name..."
+            pnpm install
+            pnpm link ../packages/galacean
+            pnpm $build_cmd
+            cd ..
+          }
+          
+          setup_repo "engine-toolkit" "b:all"
+          setup_repo "engine-lottie"
+          setup_repo "engine-spine"
-      - name: Install and Link Engine and Build for Toolkit
-        working-directory: ./engine-toolkit
-        run: |
-          pnpm install
-          pnpm link ../packages/galacean
-          pnpm b:all
-
-      - name: Install and Link Engine and Build for Lottie
-        working-directory: ./engine-lottie
-        run: |
-          pwd
-          pnpm install --dir ${{ github.workspace }}/engine-lottie
-          pnpm link ../packages/galacean
-          pnpm build
-
-      - name: Install and Link Engine and Build for Spine
-        working-directory: ./engine-spine
-        run: |
-          pnpm install
-          pnpm link ../packages/galacean
-          pnpm build

45-49: 🛠️ Refactor suggestion

Enhance repository cloning security and reliability

  1. Use authenticated git clone to avoid rate limits
  2. Add error handling for clone operations
       - name: Checkout Sub-repositories
         run: |
-          git clone https://github.com/galacean/engine-toolkit.git
-          git clone https://github.com/galacean/engine-lottie.git
-          git clone https://github.com/galacean/engine-spine.git
+          clone_repo() {
+            local repo=$1
+            echo "Cloning $repo..."
+            git clone "https://${{ secrets.GITHUB_TOKEN }}@github.com/galacean/$repo.git" || {
+              echo "Failed to clone $repo"
+              exit 1
+            }
+          }
+          
+          clone_repo "engine-toolkit"
+          clone_repo "engine-lottie"
+          clone_repo "engine-spine"

Likely invalid or redundant comment.

Comment on lines +84 to +104
- name: Bundle polyfill and engine
uses: galacean/platform-adapter@main
env:
ADAPTER_BUNDLE_SETTINGS: |
{
"polyfill": true,
"engine": [
"${{ github.workspace }}/packages/galacean/dist/module.js",
"${{ github.workspace }}/packages/xr/dist/module.js",
"${{ github.workspace }}/packages/shader-lab/dist/module.js",
"${{ github.workspace }}/packages/physics-lite/dist/module.js",
"${{ github.workspace }}/packages/physics-physx/dist/module.js",
"${{ github.workspace }}/engine-lottie/dist/module.js",
"${{ github.workspace }}/engine-spine/dist/module.js",
"${{ github.workspace }}/engine-toolkit/galacean-engine-toolkit/dist/module.js"
],
"jsWASMLoader": [
"${{ github.workspace }}/packages/physics-physx/libs/physx.release.js"
],
"outputDir": "${{ github.workspace }}/platform-adapter/dist"
}
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Use specific version tag instead of @main for platform-adapter action

The latest release of the platform-adapter is v0.0.0-alpha.2, and using @main tag is indeed risky as it can lead to unexpected behavior when the main branch changes. Replace:

uses: galacean/platform-adapter@main

with:

uses: galacean/[email protected]

Additionally, consider moving the bundle configuration to a separate JSON file in the repository for better maintainability.

🔗 Analysis chain

Verify platform adapter action version and enhance configuration

  1. Using @main tag is risky as it can change unexpectedly
  2. Consider moving the bundle configuration to a separate file for better maintainability
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check latest release of platform-adapter
gh api repos/galacean/platform-adapter/releases/latest --jq '.tag_name'

Length of output: 86

Comment on lines +39 to +44
- name: Cache Engine Package
uses: actions/cache@v3
with:
path: ./packages/galacean
key: ${{ runner.os }}-engine-${{ github.sha }}

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update cache action version and enhance cache key

  1. The actions/cache@v3 action should be updated to v4 for better compatibility with current runners.
  2. Consider adding package.json hash to the cache key for more precise invalidation.
       - name: Cache Engine Package
-        uses: actions/cache@v3
+        uses: actions/cache@v4
         with:
           path: ./packages/galacean
-          key: ${{ runner.os }}-engine-${{ github.sha }}
+          key: ${{ runner.os }}-engine-${{ github.sha }}-${{ hashFiles('**/package.json') }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Cache Engine Package
uses: actions/cache@v3
with:
path: ./packages/galacean
key: ${{ runner.os }}-engine-${{ github.sha }}
- name: Cache Engine Package
uses: actions/cache@v4
with:
path: ./packages/galacean
key: ${{ runner.os }}-engine-${{ github.sha }}-${{ hashFiles('**/package.json') }}
🧰 Tools
🪛 actionlint (1.7.4)

40-40: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

@MrKou47 MrKou47 closed this Dec 23, 2024
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