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(ci): correct github-script API calls #14442

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

dnwe
Copy link
Contributor

@dnwe dnwe commented Aug 27, 2024

User description

Description

Since V5 of github-script the Octokit context available via github no longer has REST methods directly on it, they were moved to github.rest.* instead. Update the references in delete-comments.yml job to match.

Motivation and Context

The GitHub Actions workflow will fail when it attempts to delete a comment without this change.

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

Bug fix, Enhancement


Description

  • Updated the GitHub Actions workflow to use the new github.rest.* syntax for API calls, ensuring compatibility with version 5 of github-script.
  • Modified the deleteComment and block methods to align with the updated Octokit context, preventing workflow failures.

Changes walkthrough 📝

Relevant files
Bug fix
delete-comments.yml
Update GitHub API calls to new Octokit syntax                       

.github/workflows/delete-comments.yml

  • Updated GitHub API calls to use github.rest.* syntax.
  • Changed deleteComment and block methods to use the new Octokit
    context.
  • +2/-2     

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

    @CLAassistant
    Copy link

    CLAassistant commented Aug 27, 2024

    CLA assistant check
    All committers have signed the CLA.

    Since V5 of github-script the Octokit context available via `github` no
    longer has REST methods directly on it, they were moved to
    `github.rest.*` instead. Update the references in delete-comments.yml
    job to match.
    
    Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review

    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 27, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Remove unnecessary parameter from API call to prevent potential errors

    Remove the repo parameter from the github.rest.users.block() call as it's not
    required for this API endpoint and may cause an error.

    .github/workflows/delete-comments.yml [41-45]

     await github.rest.users.block({
       owner: context.repo.owner,
    -  repo: context.repo.repo,
       user_id: userId
     });
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Removing the repo parameter from the github.rest.users.block() call is crucial as it is not required and could cause errors, thus improving the robustness of the code.

    9
    Best practice
    ✅ Use the preferred parameter for user identification in the block operation
    Suggestion Impact:The commit changed the parameter from user_id to username in the block operation, aligning with the suggestion to use the preferred parameter for user identification.

    code diff:

    -            const userId = context.payload.comment.user.id;
    -            await github.rest.users.block({
    -              owner: context.repo.owner,
    -              repo: context.repo.repo,
    -              user_id: userId
    +            const username = context.payload.comment.user.login
    +            await github.rest.orgs.blockUser({
    +              org: context.repo.owner,
    +              username: username

    Consider using the username parameter instead of user_id when blocking a user. The
    GitHub API prefers username for user identification in the block operation.

    .github/workflows/delete-comments.yml [41-45]

     await github.rest.users.block({
       owner: context.repo.owner,
    -  repo: context.repo.repo,
    -  user_id: userId
    +  username: context.payload.comment.user.login
     });
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion to use username instead of user_id aligns with GitHub API best practices for user identification, which can improve code reliability and maintainability.

    8

    dnwe added 2 commits August 27, 2024 13:58
    This is too generic a word and frequently matches against comments that
    don't need to be deleted, nor should the user be blocked as the current
    workflow will do.
    
    Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
    The existing code was calling the individual "block a user" REST
    endpoint with incorrect parameters and never would have worked. Update
    it to (presumably achieve the desired outcome) block the user from the
    owning organisation instead.
    
    Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @dnwe!

    @diemol diemol merged commit cdc57b2 into SeleniumHQ:trunk Aug 28, 2024
    3 of 4 checks passed
    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.

    3 participants