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

[js] Trying to fix JS nightly #14048

Merged
merged 1 commit into from
May 29, 2024
Merged

[js] Trying to fix JS nightly #14048

merged 1 commit into from
May 29, 2024

Conversation

diemol
Copy link
Member

@diemol diemol commented May 29, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

configuration changes, enhancement


Description

  • Added authentication token configuration to .npmrc and ~/.npmrc to ensure proper access to GitHub packages.
  • Updated registry URL for @seleniumhq packages in .npmrc and ~/.npmrc to use GitHub's npm registry.
  • Ensured always-auth is set to true in .npmrc and ~/.npmrc to enforce authentication for all npm operations.

Changes walkthrough 📝

Relevant files
Configuration changes
nightly.yml
Update npm registry and authentication settings for nightly builds

.github/workflows/nightly.yml

  • Added authentication token configuration to .npmrc and ~/.npmrc.
  • Updated registry URL for @seleniumhq packages in .npmrc and ~/.npmrc.
  • Ensured always-auth is set to true in .npmrc and ~/.npmrc.
  • +3/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @diemol diemol merged commit 036d840 into trunk May 29, 2024
    9 checks passed
    @diemol diemol deleted the fix-js-nightly branch May 29, 2024 10:20
    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are minimal and localized to configuration files, but understanding the implications of these changes requires specific knowledge about npm and GitHub package registries.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Security Risk: The use of NODE_AUTH_TOKEN in a publicly visible file (.github/workflows/nightly.yml) could potentially expose sensitive information if not properly secured.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use npm config set with the --global flag instead of appending directly to ~/.npmrc to avoid potential issues with file permissions or user-specific configurations

    To avoid potential issues with file permissions or user-specific configurations, consider
    using the --global flag with npm config set instead of appending directly to ~/.npmrc.

    .github/workflows/nightly.yml [105-109]

    -echo "//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}" >> ~/.npmrc
    -echo "@seleniumhq:registry=https://npm.pkg.github.com" >> ~/.npmrc
    -echo "always-auth=true" >> ~/.npmrc
    +npm config set //npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN} --global
    +npm config set @seleniumhq:registry https://npm.pkg.github.com --global
    +npm config set always-auth true --global
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the script by using npm config set with the --global flag, which is a more robust and system-wide approach compared to directly appending to ~/.npmrc. This avoids potential issues with file permissions and ensures consistency across environments.

    8
    Maintainability
    Remove duplicate lines that append the same configurations to both ~/.npmrc and .npmrc to ensure consistency and avoid redundancy

    To ensure consistency and avoid redundancy, remove the duplicate lines that append the
    same configurations to both ~/.npmrc and .npmrc.

    .github/workflows/nightly.yml [105-110]

    -echo "//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}" >> ~/.npmrc
     echo "//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}" >> .npmrc
    -echo "@seleniumhq:registry=https://npm.pkg.github.com" >> ~/.npmrc
     echo "@seleniumhq:registry=https://npm.pkg.github.com" >> .npmrc
    -echo "always-auth=true" >> ~/.npmrc
     echo "always-auth=true" >> .npmrc
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to remove duplicate lines is valid and improves maintainability by avoiding redundancy. However, it's less critical than the first suggestion as it does not address potential system-wide issues but rather focuses on cleaning up the script.

    7

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant