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

Next #130

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Next #130

merged 2 commits into from
Oct 23, 2024

Conversation

seeratawan01
Copy link
Member

@seeratawan01 seeratawan01 commented Oct 23, 2024

PR Type

enhancement, configuration changes


Description

  • Enhanced the CI/CD workflow by adding steps to generate and set a Release Candidate (RC) version for packages.
  • Included a new step to publish the RC SDK package to the npm registry.
  • Updated the package.json to include @types/jest in devDependencies for improved testing support.

Changes walkthrough 📝

Relevant files
Enhancement
cd-develop.yml
Enhance CI/CD workflow with RC versioning and publishing 

.github/workflows/cd-develop.yml

  • Added steps to generate and set RC version for packages.
  • Included publishing of RC SDK package to npm registry.
  • Enhanced deployment process with additional configurations.
  • +77/-37 
    Dependencies
    package.json
    Update development dependencies for testing                           

    packages/javascript-sdk/package.json

    • Added @types/jest to devDependencies.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @seeratawan01 seeratawan01 merged commit e447dee into develop Oct 23, 2024
    1 of 2 checks passed
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🏅 Score: 75
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The script writes the npm authentication token directly into the .npmrc file which could potentially expose sensitive information if the file is not properly secured.

    ⚡ Recommended focus areas for review

    Possible Bug
    The script uses 'npm install -g pnpm' which might not be necessary if 'pnpm' is already globally installed. Consider checking if 'pnpm' is installed before attempting to install it again.

    Security Concern
    The script writes the npm authentication token directly into the .npmrc file which could potentially expose sensitive information if the file is not properly secured.

    Code feedback:
    relevant file.github/workflows/cd-develop.yml
    suggestion      

    Consider using environment variables for Node.js version to enhance flexibility and maintainability of the workflow script. [important]

    relevant linenode-version: '20'

    relevant file.github/workflows/cd-develop.yml
    suggestion      

    It's recommended to add error handling or a verification step after each critical operation, such as after setting the package version or publishing the package, to ensure the step was successful before proceeding. [important]

    relevant linenpm version $RC_VERSION --no-git-tag-version

    relevant file.github/workflows/cd-develop.yml
    suggestion      

    To improve the security, consider using a more secure method to handle npm tokens, such as using GitHub's encrypted secrets directly in the npm publish command without writing them to a file. [important]

    relevant lineecho "//registry.npmjs.org/:_authToken=${{ secrets.NODE_AUTH_TOKEN }}" > .npmrc

    relevant file.github/workflows/cd-develop.yml
    suggestion      

    Instead of manually iterating over packages and setting versions, consider using a tool or script that handles version updates in a more automated and error-prone manner. [medium]

    relevant linefor package in packages/*; do

    relevant file.github/workflows/cd-develop.yml
    suggestion      

    To ensure that the 'pnpm build' command succeeds before proceeding, consider adding a check to confirm the build status. [medium]

    relevant linerun: pnpm build

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Remove unnecessary and potentially insecure tool installations

    Remove the redundant installation of npm-cli-login as it is not used in the workflow
    and could potentially introduce security risks.

    .github/workflows/cd-develop.yml [39-40]

     - name: Ignore Workspace root check
    -  run: npm install -g npm-cli-login
    +  run: echo "Skipping workspace root check"
    Suggestion importance[1-10]: 9

    Why: Removing the installation of npm-cli-login, which is not used in the workflow, reduces potential security risks and unnecessary dependencies, making this a high-impact suggestion.

    9
    Best practice
    Maintain consistency in package management by using pnpm for publishing

    Use pnpm publish instead of npm publish to maintain consistency with the rest of the
    workflow that utilizes pnpm.

    .github/workflows/cd-develop.yml [75-85]

     - name: Publishing RC SDK package
       run: |
         for package in packages/*; do
         if [ -d "$package" ]; then
             cd $package
             echo "Publishing $package RC SDK package"
             echo  "//registry.npmjs.org/:_authToken=${{ secrets.NODE_AUTH_TOKEN }}" > .npmrc
    -        npm publish --tag rc --no-workspaces --access=public            
    +        pnpm publish --tag rc --no-workspaces --access=public            
             cd - > /dev/null
         fi
         done
    Suggestion importance[1-10]: 8

    Why: Switching from npm publish to pnpm publish maintains consistency with the rest of the workflow, which uses pnpm, potentially reducing errors and improving maintainability.

    8
    Improve the installation process of pnpm

    Replace the usage of npm install -g pnpm with pnpm setup to ensure a more reliable
    and up-to-date installation of pnpm.

    .github/workflows/cd-develop.yml [33-34]

     - name: Install pnpm
    -  run: npm install -g pnpm
    +  run: pnpm setup
    Suggestion importance[1-10]: 7

    Why: The suggestion to use pnpm setup instead of npm install -g pnpm is valid as it ensures a more reliable and up-to-date installation of pnpm, aligning with best practices for package management.

    7
    Ensure compatibility and stability by using the LTS version of Node.js

    Ensure that the node-version specified in the setup-node action is compatible with
    the latest stable release or aligns with the project's requirements.

    .github/workflows/cd-develop.yml [27-31]

     - name: Setup Node.js
       uses: actions/setup-node@v4
       with:
         always-auth: true
    -    node-version: '20'
    +    node-version: '16' # Assuming LTS version is preferred
    Suggestion importance[1-10]: 5

    Why: While using an LTS version of Node.js can enhance stability, the suggestion assumes a preference without context from the PR. The current version might be intentional, so this suggestion has moderate impact.

    5

    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