Skip to content

ci(mono-pub): run publish job from LTS node #28

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.

Sign up for GitHub

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

Closed
SavelevMatthew wants to merge 2 commits into from
Closed

ci(mono-pub): run publish job from LTS node #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff98cfd
ci(mono-pub): run publish job from LTS node
SavelevMatthew Dec 8, 2024
4ac8b63
ci(mono-pub): fix publishing job
SavelevMatthew Dec 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/nodejs.packages.ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ jobs:
steps:
- name: Checkout code
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Setup Node.js environment
uses: actions/setup-node@v4
Expand Down
38 changes: 38 additions & 0 deletions .github/workflows/nodejs.packages.debug.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
name: Publish NPM packages

permissions:
contents: write
id-token: write

Comment on lines +3 to +6
Copy link

@coderabbitai coderabbitai bot Dec 8, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Permissions are unnecessarily broad for the debug workflow

The id-token: write permission is not used in this workflow as there's no OIDC authentication or token exchange. Additionally, contents: write is excessive since the debug workflow only reads git tags. Suggested permissions:

permissions:
  contents: read
  • .github/workflows/nodejs.packages.debug.yml: Remove id-token: write and downgrade contents to read
🔗 Analysis chain

Review and restrict permissions scope

The workflow has broad write permissions that should be reviewed:

  • contents: write might be too permissive if only needed for tagging
  • id-token: write suggests OIDC usage, but no OIDC configuration is present in the workflow
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any OIDC-related configurations in other workflow files
rg -l "id-token|oidc|permissions" .github/workflows/

Length of output: 185

Script:

#!/bin/bash
# Check the content of these workflow files to understand the OIDC usage
for file in nodejs.packages.{publish,debug,ci}.yml; do
    echo "=== $file ==="
    cat ".github/workflows/$file" 2>/dev/null
done

Length of output: 3780


on:
push:
branches: [ "main" ]
pull_request:
branches: [ "main" ]

Comment on lines +8 to +13
Copy link

@coderabbitai coderabbitai bot Dec 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Reconsider workflow triggers for package publishing

Running the publish workflow on PR events could be risky:

  • Could lead to duplicate runs (PR + push)
  • Package publishing should typically only happen on merge to main

Consider this configuration instead:

 on:
   push:
     branches: [ "main" ]
-  pull_request:
-    branches: [ "main" ]
📝 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
on:
push:
branches: [ "main" ]
pull_request:
branches: [ "main" ]
on:
push:
branches: [ "main" ]

jobs:
publish:
runs-on: ubuntu-22.04

steps:
- name: Checkout code
uses: actions/checkout@v4
with:
fetch-depth: 0

- name: Setup Node.js environment
uses: actions/setup-node@v4
with:
cache: 'yarn'

Comment on lines +24 to +28
Copy link

@coderabbitai coderabbitai bot Dec 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Specify Node.js LTS version

As per PR objective to run from LTS node, the Node.js version should be explicitly specified.

 - name: Setup Node.js environment
   uses: actions/setup-node@v4
   with:
+    node-version: 'lts/*'
     cache: 'yarn'
📝 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: Setup Node.js environment
uses: actions/setup-node@v4
with:
cache: 'yarn'
- name: Setup Node.js environment
uses: actions/setup-node@v4
with:
node-version: 'lts/*'
cache: 'yarn'

- name: Install dependencies
run: yarn --immutable

# NOTE: Mono-pub uses itself to publish its packages, so we need to prebuild them
- name: Build packages
run: yarn build

- name: Debug 1
run: |
git tag --merged
Comment on lines +36 to +38
Copy link

@coderabbitai coderabbitai bot Dec 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Debug step appears incomplete

The debug step only shows merged tags but doesn't provide meaningful debugging information. Consider:

  • Adding more git status information
  • Logging workspace state
  • Checking package versions
 - name: Debug 1
   run: |
-    git tag --merged
+    git status
+    git tag --merged
+    yarn workspaces list
+    yarn workspaces foreach npm version
📝 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: Debug 1
run: |
git tag --merged
- name: Debug 1
run: |
git status
git tag --merged
yarn workspaces list
yarn workspaces foreach npm version

Loading