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

Updated renovate.json to add correct commitMessagePrefix #14709

Merged
merged 19 commits into from
Dec 19, 2024

Conversation

DineshKumarRA
Copy link
Contributor

@DineshKumarRA DineshKumarRA commented Nov 3, 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

Updated renovate.json to add correct commitMessagePrefix based on packages. Have also increased the prConcurrentLimit to 15 as I think 5 is too low for a big project. But happy to change this back.

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

enhancement, configuration changes


Description

  • Added a new CI workflow specifically for renovate branches, including steps for repinning dependencies and running format scripts.
  • Updated the renovate configuration to include specific commit message prefixes and labels for various package managers.
  • Increased the prConcurrentLimit from 5 to 15 to allow more concurrent PRs.
  • Modified existing CI workflows to exclude renovate branches from format and test jobs.

Changes walkthrough 📝

Relevant files
Configuration changes
ci-rbe.yml
Exclude renovate branches from CI format and test jobs     

.github/workflows/ci-rbe.yml

  • Added condition to exclude renovate branches from format and test
    jobs.
  • +2/-2     
    ci-renovate-rbe.yml
    Add CI workflow for renovate branches                                       

    .github/workflows/ci-renovate-rbe.yml

  • Added a new CI workflow specifically for renovate branches.
  • Included steps for repinning dependencies and running format scripts.
  • +53/-0   
    renovate.json
    Update renovate configuration for package management         

    renovate.json

  • Updated commit message prefixes and labels for various package
    managers.
  • Increased prConcurrentLimit from 5 to 15.
  • +62/-16 

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

    @CLAassistant
    Copy link

    CLAassistant commented Nov 3, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 3, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit ed666f7)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The new CI workflow for renovate branches (.github/workflows/ci-renovate-rbe.yml) uses a secret named SELENIUM_CI_TOKEN for pushing changes. While using secrets is a common practice, it's important to ensure that this token has the minimum required permissions and is properly secured. Review the permissions granted to this token and confirm that it's not exposing more access than necessary.

    ⚡ Recommended focus areas for review

    Potential Security Risk
    The new workflow uses a secret (SELENIUM_CI_TOKEN) for pushing changes. Ensure this token has the minimum required permissions and is properly secured.

    Configuration Complexity
    The renovate.json file has become quite complex with many package rules. This might make future maintenance more difficult. Consider if some rules can be consolidated or simplified.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 3, 2024

    PR Code Suggestions ✨

    Latest suggestions up to ed666f7
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    ✅ Improve the reliability of detecting and pushing changes in the workflow
    Suggestion Impact:The commit removed the use of the environment variable 'code_changes' and directly pushed changes after committing, aligning with the suggestion to use a more specific condition.

    code diff:

    -            echo "code_changes=true" >> $GITHUB_ENV
    +            git push

    Consider using a more specific condition for pushing changes, such as checking the
    exit code of the git commit command, instead of relying on an environment variable
    set in a previous step.

    .github/workflows/ci-renovate-rbe.yml [32-35]

    -- name: Push changes
    -  if: ${{ env.code_changes }} == 'true'
    -  uses: ad-m/github-push-action@master
    +- name: Commit and push changes
    +  run: |
    +    if [ -n "$(git status --porcelain)" ]; then
    +      git config --local user.email "${{ env.GIT_USER_EMAIL }}"
    +      git config --local user.name "${{ env.GIT_USER_NAME }}"
    +      git add ./java/maven_install.json
    +      git commit -m 'Repin maven dependencies'
    +      git push
    +    fi
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion offers a more robust approach to detecting and pushing changes, which could prevent potential issues and improve the overall reliability of the workflow.

    8
    Fetch the complete Git history when checking out the repository

    Use the fetch-depth: 0 option when checking out the repository to ensure all history
    is fetched, which may be necessary for certain operations or comparisons.

    .github/workflows/ci-renovate-rbe.yml [15]

     - name: Checkout Repository
       uses: actions/checkout@v2
    +  with:
    +    fetch-depth: 0
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Fetching the complete Git history can be beneficial for certain operations, but it may not be necessary for this specific workflow and could potentially slow down the checkout process.

    5
    Enhancement
    ✅ Update the GitHub Actions checkout action to a more recent version
    Suggestion Impact:The checkout action was updated from 'v2' to 'v4', which aligns with the suggestion to use a more recent version.

    code diff:

    -        uses: actions/checkout@v2
    +        uses: actions/checkout@v4

    Consider using a more recent version of the checkout action, such as 'v3' or 'v4',
    to benefit from the latest features and bug fixes.

    .github/workflows/ci-renovate-rbe.yml [15]

     - name: Checkout Repository
    -  uses: actions/checkout@v2
    +  uses: actions/checkout@v3
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Updating to a more recent version of the checkout action can provide bug fixes and new features, potentially improving the workflow's reliability and performance.

    7
    Maintainability
    Use environment variables for git configuration values to improve maintainability

    Use environment variables for repeated values like the email and name in the git
    config commands to improve maintainability.

    .github/workflows/ci-renovate-rbe.yml [25-26]

    -git config --local user.email "[email protected]"
    -git config --local user.name "Selenium CI Bot"
    +env:
    +  GIT_USER_EMAIL: "[email protected]"
    +  GIT_USER_NAME: "Selenium CI Bot"
    +steps:
    +  # ... other steps ...
    +  - name: Commit files
    +    run: |
    +      export CHANGES=$(git status -s)
    +      if [ -n "$CHANGES" ]; then
    +        git config --local user.email "${{ env.GIT_USER_EMAIL }}"
    +        git config --local user.name "${{ env.GIT_USER_NAME }}"
    Suggestion importance[1-10]: 6

    Why: Using environment variables for repeated values improves maintainability and reduces the risk of inconsistencies, but the impact is moderate as it affects only a small part of the workflow.

    6

    💡 Need additional feedback ? start a PR chat


    Previous suggestions

    ✅ Suggestions up to commit 8463b0f
    CategorySuggestion                                                                                                                                    Score
    Best practice
    Define a schedule for updates to manage timing and reduce potential disruptions

    Consider adding a schedule field to the configuration to define when updates should
    be performed, helping to manage the timing of dependency updates and potentially
    reducing disruptions during critical periods.

    renovate.json [1-5]

     {
       "$schema": "https://docs.renovatebot.com/renovate-schema.json",
       "extends": ["config:recommended"],
       "labels": ["dependencies"],
    +  "schedule": ["after 10pm and before 5am every weekday", "every weekend"],
       "packageRules": [
    Suggestion importance[1-10]: 9

    Why: Adding a schedule field is a crucial enhancement that can greatly reduce disruptions by controlling when updates occur. This suggestion addresses an important aspect of dependency management that was missing in the original PR.

    9
    Enhancement
    Group related updates to reduce the number of PRs and streamline the update process

    Consider adding a groupName field to each package rule to group related updates
    together, potentially reducing the number of PRs and making the update process more
    manageable.

    renovate.json [16-20]

     {
       "matchManagers": ["maven", "github-actions"],
       "commitMessagePrefix": "[java]",
    -  "labels": ["c-java"]
    +  "labels": ["c-java"],
    +  "groupName": "Java dependencies"
     },
    Suggestion importance[1-10]: 8

    Why: Introducing a groupName field can significantly improve the efficiency of the update process by reducing the number of PRs. This change could have a substantial positive impact on project maintenance and workflow.

    8
    ✅ Separate package managers into individual entries for more precise control
    Suggestion Impact:The commit separated "pip_requirements" and "pip_setup" into individual entries, partially implementing the suggestion for more granular control of Python-related package managers.

    code diff:

    +      "matchManagers": [ "pip_requirements", "pip_setup" ],
    +      "commitMessagePrefix": "[py]",
    +      "labels": [ "dependencies", "c-py" ]
    +    },
    +    {
    +      "matchPackageNames": [ "rules_python" ],
    +      "commitMessagePrefix": "[py]",
    +      "labels": [ "dependencies", "c-py" ]

    Consider using more specific manager names for Python-related package managers.
    Instead of using a catch-all "pip_requirements, pip_setup, pep621", you could
    separate them into individual entries for more granular control.

    renovate.json [26-30]

     {
    -  "matchManagers": ["pip_requirements", "pip_setup", "pep621"],
    +  "matchManagers": ["pip_requirements"],
    +  "commitMessagePrefix": "[py]",
    +  "labels": ["c-py"]
    +},
    +{
    +  "matchManagers": ["pip_setup"],
    +  "commitMessagePrefix": "[py]",
    +  "labels": ["c-py"]
    +},
    +{
    +  "matchManagers": ["pep621"],
       "commitMessagePrefix": "[py]",
       "labels": ["c-py"]
     },
    Suggestion importance[1-10]: 5

    Why: This suggestion offers improved granularity in managing Python-related package managers, which could be beneficial for more precise control. However, the current grouping might be intentional for simplicity, so the impact is moderate.

    5
    Maintainability
    Add descriptions to package rules for improved clarity and maintainability

    Consider adding a description field to each package rule to provide more context
    about why specific managers are grouped together or why certain prefixes and labels
    are used.

    renovate.json [6-10]

     {
    +  "description": "Rules for Bazel-related managers affecting multiple languages",
       "matchManagers": ["bazel", "bazel-module", "bazelisk"],
       "commitMessagePrefix": "[dotnet][java][js][py][rb][rust]",
       "labels": ["c-build"]
     },
    Suggestion importance[1-10]: 7

    Why: Adding descriptions to package rules significantly enhances code readability and maintainability. It provides valuable context for future developers, making it easier to understand and modify the configuration.

    7

    @DineshKumarRA DineshKumarRA marked this pull request as draft November 3, 2024 22:54
    @VietND96
    Copy link
    Member

    VietND96 commented Nov 3, 2024

    For renovate's PR, sometimes we have to run format manually in PR branch.
    Sometimes in commits (e.g preparation for release), bulk of dependencies are updated via bazel run.
    What is your view on renovate's PR? @diemol, @p0deje, @harsha509

    @p0deje
    Copy link
    Member

    p0deje commented Nov 4, 2024

    @VietND96 Ideally we'd wire up the format + dependencies update command to automatically run on Renovate PRs and commit to the same branch so that maintainers only need to review PR and merge it. If the whole automation just works, we can enable auto-merge for renovate PRs as long as CI passes.

    @DineshKumarRA
    Copy link
    Contributor Author

    @VietND96 @p0deje I can create a separate workflow for Renovate PRs to pin the maven dependencies and commit using the Selenium CI bot. Do you also want to add format script at the end before the commit?

    @DineshKumarRA DineshKumarRA marked this pull request as ready for review November 5, 2024 12:10
    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 5, 2024

    Persistent review updated to latest commit ed666f7

    @DineshKumarRA
    Copy link
    Contributor Author

    @VietND96 @p0deje @diemol This is now ready for review

    .github/workflows/ci-renovate-rbe.yml Outdated Show resolved Hide resolved
    @DineshKumarRA DineshKumarRA requested a review from p0deje November 5, 2024 23:59
    @DineshKumarRA
    Copy link
    Contributor Author

    @p0deje @VietND96 There is "maven_install.json" and sometimes "Cargo.Bazel.lock" being generating when running the repin. Does both the files need to be committed into the branch?
    If yes, then I can change the git add command to 'git add .' rather than "git add ./java/maven_install.json" to add all the changed files

    @p0deje
    Copy link
    Member

    p0deje commented Nov 15, 2024

    @DineshKumarRA Yes, we need to commit both maven_install.json and Cargo.Bazel.lock.

    @DineshKumarRA
    Copy link
    Contributor Author

    @p0deje @VietND96 This is now ready. JS dependency is still failing due to aspect-build/rules_js#1445. But others are all fixed now.

    @DineshKumarRA
    Copy link
    Contributor Author

    @VietND96 @p0deje @diemol Have updated the repin dependencies for all languages. It is now ready for review.

    @p0deje p0deje merged commit 7ee9d16 into SeleniumHQ:trunk Dec 19, 2024
    9 checks passed
    @DineshKumarRA DineshKumarRA deleted the update-renovate-json branch December 19, 2024 22:00
    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.

    4 participants