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

feat: handle gitlab MR draft status #1165

Merged
merged 4 commits into from
Aug 23, 2024

Conversation

paolomainardi
Copy link
Contributor

@paolomainardi paolomainardi commented Aug 22, 2024

User description

closes #1160

As reported in the issue, this MR adds the possibility to skip MR in draft status, default is false when enabled every command is logged and discarded.


PR Type

Enhancement


Description

  • Added functionality to handle GitLab merge requests (MRs) in draft status
  • Implemented logic to skip processing of draft MRs:
    • For newly opened or reopened MRs
    • For comments on existing MRs
  • Added logging to inform when a draft MR is being skipped
  • This change allows for more efficient processing by ignoring MRs that are not ready for review

Changes walkthrough 📝

Relevant files
Enhancement
gitlab_webhook.py
Handle GitLab merge request draft status                                 

pr_agent/servers/gitlab_webhook.py

  • Added logic to check if a merge request is in draft status
  • Implemented skipping of draft merge requests for both new MRs and
    comments on existing MRs
  • Logged information about skipping draft MRs
  • +11/-0   

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

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added the enhancement New feature or request label Aug 22, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 No multiple PR themes
    ⚡ Key issues to review

    Code Duplication
    There's some duplication in the logic for handling draft MRs in both the 'merge_request' and 'note' event handlers. Consider extracting this logic into a separate function to improve maintainability.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Aug 22, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    ✅ Extract draft MR skipping logic into a separate function to improve code organization
    Suggestion Impact:The suggestion was implemented by creating a new function 'should_skip_mr' that encapsulates the logic for checking if a merge request is a draft and should be skipped.

    code diff:

    +def should_skip_mr(draft: bool, mr_url: str):
    +    skip_draft_mr = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
    +    if draft and skip_draft_mr:
    +        get_logger().info(f"Skipping draft MR: {mr_url}")
    +        return True
    +    return False
     
     @router.post("/webhook")
     async def gitlab_webhook(background_tasks: BackgroundTasks, request: Request):
    @@ -130,8 +136,7 @@
                 url = data['object_attributes'].get('url')
                 draft = data['object_attributes'].get('draft')
     
    -            if draft and skip_draft_mr:
    -                get_logger().info(f"Skipping draft MR: {url}")
    +            if should_skip_mr(draft, url):
                     return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
     
                 get_logger().info(f"New merge request: {url}")
    @@ -143,8 +148,7 @@
                     draft = mr.get('draft')
     
                     get_logger().info(f"A comment has been added to a merge request: {url}")
    -                if draft and skip_draft_mr:
    -                    get_logger().info(f"Skipping draft MR: {url}")
    +                if should_skip_mr(draft, url):
                         return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))

    Consider extracting the logic for checking if a merge request is a draft and should
    be skipped into a separate function. This will improve code readability and reduce
    duplication.

    pr_agent/servers/gitlab_webhook.py [133-135]

    -if draft and skip_draft_mr:
    -    get_logger().info(f"Skipping draft MR: {url}")
    -    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    +if should_skip_draft_mr(draft, skip_draft_mr, url):
    +    return SUCCESS_RESPONSE
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Extracting the draft MR skipping logic into a separate function would significantly improve code readability and reduce duplication, as this logic is used in multiple places.

    8
    Simplify the condition for checking bot-like sender names using a list comprehension

    Consider using f-strings consistently throughout the code. Replace the string
    concatenation with an f-string for improved readability and consistency.

    pr_agent/servers/gitlab_webhook.py [122]

    -if 'codium' in sender_name or 'bot_' in sender_name or 'bot-' in sender_name or '_bot' in sender_name or '-bot' in sender_name:
    +if any(bot_indicator in sender_name for bot_indicator in ['codium', 'bot_', 'bot-', '_bot', '-bot']):
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggested improvement using a list comprehension would make the code more concise and potentially more efficient. It's a good enhancement but not addressing a critical issue.

    7
    Maintainability
    ✅ Use a constant for repeated success responses to improve code maintainability
    Suggestion Impact:The commit implemented the suggestion by creating a function `create_success_response()` that returns a standardized success response. This function is then used throughout the code to replace repeated JSONResponse calls.

    code diff:

    +def create_success_response():
    +    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))

    Consider using a constant for the 'success' message to avoid repetition and improve
    maintainability. Define a constant at the module level and use it in all return
    statements.

    pr_agent/servers/gitlab_webhook.py [124]

    -return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    +return SUCCESS_RESPONSE
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a constant for repeated success responses would improve maintainability and reduce code duplication. However, it's not a critical issue affecting functionality.

    7
    Best practice
    ✅ Use a more descriptive variable name to improve code readability
    Suggestion Impact:The suggestion was implemented, but with a different approach. Instead of renaming the variable, a new function 'should_skip_draft_mr' was created that encapsulates the logic for skipping draft merge requests.

    code diff:

    +def should_skip_draft_mr(draft: bool, mr_url: str):
    +    skip_draft_mr = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
    +    if draft and skip_draft_mr:
    +        get_logger().info(f"Skipping draft MR: {mr_url}")
    +        return True
    +    return False

    Consider using a more descriptive variable name for skip_draft_mr. A name like
    should_skip_draft_mrs would make the boolean's purpose clearer.

    pr_agent/servers/gitlab_webhook.py [127]

    -skip_draft_mr = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
    +should_skip_draft_mrs = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a more descriptive variable name would improve code readability, but the current name is not severely unclear. It's a good practice but not crucial.

    6

    @paolomainardi
    Copy link
    Contributor Author

    /review

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 92
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 Multiple PR themes

    Sub-PR theme: Implement draft MR handling logic

    Relevant files:

    • pr_agent/servers/gitlab_webhook.py

    Sub-PR theme: Add configuration option for skipping draft MRs

    Relevant files:

    • pr_agent/settings/configuration.toml

    ⚡ Key issues to review

    Possible Bug
    The skip_draft_mr variable is defined but not used in the function. It might be better to use the should_skip_mr function directly.

    @paolomainardi
    Copy link
    Contributor Author

    /improve

    1 similar comment
    @paolomainardi
    Copy link
    Contributor Author

    /improve

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Aug 22, 2024

    PR Code Suggestions ✨

    Latest suggestions up to ee15a8a

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract bot user check into a separate function for better code organization

    Consider extracting the bot user check into a separate function to improve code
    readability and maintainability.

    pr_agent/servers/gitlab_webhook.py [131-134]

    -sender_name = data.get("user", {}).get("name", "unknown").lower()
    -if 'codium' in sender_name or 'bot_' in sender_name or 'bot-' in sender_name or '_bot' in sender_name or '-bot' in sender_name:
    +def is_bot_user(sender_name: str) -> bool:
    +    bot_indicators = ['codium', 'bot_', 'bot-', '_bot', '-bot']
    +    return any(indicator in sender_name.lower() for indicator in bot_indicators)
    +
    +sender_name = data.get("user", {}).get("name", "unknown")
    +if is_bot_user(sender_name):
         get_logger().info(f"Skipping bot user: {sender_name}")
         return create_success_response()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code organization and reusability by extracting a complex condition into a separate function.

    7
    Enhancement
    ✅ Simplify the boolean logic in the function to improve readability
    Suggestion Impact:The suggestion was implemented, but with a different approach. The function was removed entirely and its logic was integrated directly into the main webhook handler.

    code diff:

    -def should_skip_draft_mr(draft: bool, mr_url: str):
    -    skip_draft_mr = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
    -    if draft and skip_draft_mr:
    -        get_logger().info(f"Skipping draft MR: {mr_url}")
    -        return True
    -    return False
     
     @router.post("/webhook")
     async def gitlab_webhook(background_tasks: BackgroundTasks, request: Request):
    @@ -100,7 +89,8 @@
                 secret = secret_provider.get_secret(request_token)
                 if not secret:
                     get_logger().warning(f"Empty secret retrieved, request_token: {request_token}")
    -                return create_unauthorized_response()
    +                return JSONResponse(status_code=status.HTTP_401_UNAUTHORIZED,
    +                                    content=jsonable_encoder({"message": "unauthorized"}))
                 try:
                     secret_dict = json.loads(secret)
                     gitlab_token = secret_dict["gitlab_token"]
    @@ -109,19 +99,19 @@
                     context["settings"].gitlab.personal_access_token = gitlab_token
                 except Exception as e:
                     get_logger().error(f"Failed to validate secret {request_token}: {e}")
    -                return create_unauthorized_response()
    +                return JSONResponse(status_code=status.HTTP_401_UNAUTHORIZED, content=jsonable_encoder({"message": "unauthorized"}))
             elif get_settings().get("GITLAB.SHARED_SECRET"):
                 secret = get_settings().get("GITLAB.SHARED_SECRET")
                 if not request.headers.get("X-Gitlab-Token") == secret:
                     get_logger().error("Failed to validate secret")
    -                return create_unauthorized_response()
    +                return JSONResponse(status_code=status.HTTP_401_UNAUTHORIZED, content=jsonable_encoder({"message": "unauthorized"}))
             else:
                 get_logger().error("Failed to validate secret")
    -            return create_unauthorized_response()
    +            return JSONResponse(status_code=status.HTTP_401_UNAUTHORIZED, content=jsonable_encoder({"message": "unauthorized"}))
             gitlab_token = get_settings().get("GITLAB.PERSONAL_ACCESS_TOKEN", None)
             if not gitlab_token:
                 get_logger().error("No gitlab token found")
    -            return create_unauthorized_response()
    +            return JSONResponse(status_code=status.HTTP_401_UNAUTHORIZED, content=jsonable_encoder({"message": "unauthorized"}))
     
             get_logger().info("GitLab data", artifact=data)
             sender = data.get("user", {}).get("username", "unknown")
    @@ -131,28 +121,30 @@
             sender_name = data.get("user", {}).get("name", "unknown").lower()
             if 'codium' in sender_name or 'bot_' in sender_name or 'bot-' in sender_name or '_bot' in sender_name or '-bot' in sender_name:
                 get_logger().info(f"Skipping bot user: {sender_name}")
    -            return create_success_response()
    +            return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
     
             log_context["sender"] = sender
    +        should_skip_draft = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
             if data.get('object_kind') == 'merge_request' and data['object_attributes'].get('action') in ['open', 'reopen']:
                 url = data['object_attributes'].get('url')
                 draft = data['object_attributes'].get('draft')
    -
    -            if should_skip_draft_mr(draft, url):
    -                return create_success_response()
    -
                 get_logger().info(f"New merge request: {url}")
    +
    +            if draft and should_skip_draft:
    +                get_logger().info(f"Skipping draft MR: {url}")
    +                return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))

    The should_skip_draft_mr function could be simplified by returning the boolean
    expression directly, without using an if-else statement.

    pr_agent/servers/gitlab_webhook.py [83-88]

     def should_skip_draft_mr(draft: bool, mr_url: str):
         skip_draft_mr = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
    -    if draft and skip_draft_mr:
    +    should_skip = draft and skip_draft_mr
    +    if should_skip:
             get_logger().info(f"Skipping draft MR: {mr_url}")
    -        return True
    -    return False
    +    return should_skip
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion simplifies the logic and improves readability of the function, which is a good practice for maintainability.

    6
    Best practice
    Use a constant for the success message to improve maintainability

    Consider using a constant for the success status code (200) instead of hardcoding
    it. This improves maintainability and readability.

    pr_agent/servers/gitlab_webhook.py [26-27]

     def create_success_response():
         return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
     
    +SUCCESS_MESSAGE = "success"
    +
    +def create_success_response():
    +    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": SUCCESS_MESSAGE}))
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion improves code maintainability, but the impact is relatively minor as it only affects a single, simple function.

    5
    Use a more descriptive variable name for better code readability

    Consider using a more descriptive variable name for mr to improve code readability.
    For example, merge_request would be more clear.

    pr_agent/servers/gitlab_webhook.py [147-150]

     if 'merge_request' in data:
    -    mr = data['merge_request']
    -    url = mr.get('url')
    -    draft = mr.get('draft')
    +    merge_request = data['merge_request']
    +    url = merge_request.get('url')
    +    draft = merge_request.get('draft')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While using more descriptive variable names is good practice, the impact here is minimal as the current name 'mr' is already somewhat clear in context.

    4

    Previous suggestions

    ✅ Suggestions up to commit 978c445
    CategorySuggestion                                                                                                                                    Score
    Maintainability
    ✅ Rename function to better reflect its specific purpose
    Suggestion Impact:The function name was changed from 'should_skip_mr' to 'should_skip_draft_mr' as suggested

    code diff:

    -def should_skip_mr(draft: bool, mr_url: str):
    +def should_skip_draft_mr(draft: bool, mr_url: str):

    Consider using a more descriptive name for the should_skip_mr function, such as
    should_skip_draft_mr, to better reflect its specific purpose of checking if a draft
    merge request should be skipped.

    pr_agent/servers/gitlab_webhook.py [78-83]

    -def should_skip_mr(draft: bool, mr_url: str):
    +def should_skip_draft_mr(draft: bool, mr_url: str):
         skip_draft_mr = get_settings().get("GITLAB.SKIP_DRAFT_MR", False)
         if draft and skip_draft_mr:
             get_logger().info(f"Skipping draft MR: {mr_url}")
             return True
         return False
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and maintainability by proposing a more descriptive function name that accurately reflects its purpose.

    7
    ✅ Extract repeated code into a separate function to reduce duplication
    Suggestion Impact:The suggestion was implemented by creating two new functions: create_success_response() and create_unauthorized_response(). These functions were then used throughout the code to replace repeated JSONResponse creations.

    code diff:

    +def create_success_response():
    +    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    +
    +def create_unauthorized_response():
    +    return JSONResponse(status_code=status.HTTP_401_UNAUTHORIZED, content=jsonable_encoder({"message": "unauthorized"}))

    Consider extracting the repeated JSONResponse creation into a separate function to
    reduce code duplication and improve maintainability.

    pr_agent/servers/gitlab_webhook.py [139-143]

    +def create_success_response():
    +    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    +
     if should_skip_mr(draft, url):
    -    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    +    return create_success_response()
     
     get_logger().info(f"New merge request: {url}")
     await _perform_commands_gitlab("pr_commands", PRAgent(), url, log_context)
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves code maintainability by reducing duplication, but it's a minor optimization that doesn't address a critical issue.

    6
    Best practice
    Use a constant for repeated string values to improve maintainability

    Consider using a constant for the success message "success" to improve
    maintainability and reduce the risk of typos in multiple occurrences.

    pr_agent/servers/gitlab_webhook.py [139-140]

    +SUCCESS_MESSAGE = "success"
    +
     if should_skip_mr(draft, url):
    -    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    +    return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": SUCCESS_MESSAGE}))
     
    Suggestion importance[1-10]: 4

    Why: While using a constant can improve maintainability, the benefit is minimal in this case as the string is only used twice in the visible code.

    4

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Aug 22, 2024

    @paolomainardi
    simplify and shorten this PR.
    I prefer not to do 'refactoring' of some sort and logic change in the same PR.
    Also, there is no need for configuration control for something that obvious. dead configurations are a burden.

    The PR should be four lines. something like:

    draft = data['object_attributes'].get('draft')
    
    if draft:
       get_logger().debug(...)
        return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"}))
    

    @paolomainardi
    Copy link
    Contributor Author

    Ok @mrT23, I simplified it as requested and removed the refactoring part.

    @paolomainardi paolomainardi force-pushed the feature/1160_gitlab_mr_draft_skip branch from 055c4cd to ffaf5d5 Compare August 22, 2024 15:29
    @@ -230,6 +230,7 @@ push_commands = [
    "/describe",
    "/review --pr_reviewer.num_code_suggestions=0",
    ]
    skip_draft_mr = false
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    remove this. it's a dead configuration. should be 'true' always
    (and putting something by default as 'False' is like saying that no one will ever use it. We do want to use it)

    Copy link
    Contributor Author

    @paolomainardi paolomainardi Aug 22, 2024

    Choose a reason for hiding this comment

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

    I don't know what do you mean by "dead configuration", there are tons of "false" in the default settings file, maybe I am missing something.

    This line serves to:

    1. Let the users know that configuration exists and that the default is false. Otherwise where are documented ?
    2. If we set it to true it means introducing a breaking change, as now draft requests are handled as the others. I did it on purpose.

    Copy link
    Collaborator

    @mrT23 mrT23 Aug 22, 2024

    Choose a reason for hiding this comment

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

    dead configuration means something so exotic that 99.9% of PR-agent users won't ever change.
    And in addition, here you have (for a 'dead configuration') a wrong default value.

    A change is for the entire community. And its usage should be from the viewpoint of the entire community, and the entire code repo.

    Since the correct behaviour is to avoid draft PRs (this is already done, without config, in github) we should do it also in Gitlab.

    if possible and logical, the best configuration is no configuration.

    Copy link
    Contributor Author

    @paolomainardi paolomainardi Aug 22, 2024

    Choose a reason for hiding this comment

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

    So, to ensure I understood it well, are you proposing to always skip draft MR without having any configuration toggle?

    There could be use cases where you want it enabled, and dropping it entirely seems a bit drastic.

    I still don't understand why default to false should be a "wrong value."

    Copy link
    Contributor Author

    @paolomainardi paolomainardi Aug 22, 2024

    Choose a reason for hiding this comment

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

    @mrT23 anyway, just committed a version without any configuration toggle, we always skip it when the MR is in draft status.

    @paolomainardi
    Copy link
    Contributor Author

    /describe

    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Description updated to latest commit (8793f8d)

    @paolomainardi
    Copy link
    Contributor Author

    /review

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 No relevant tests
    🔒 No security concerns identified
    🔀 No multiple PR themes
    ⚡ Key issues to review

    Code Duplication
    The draft checking logic is duplicated for both new merge requests and comments. Consider extracting this logic into a separate function to improve maintainability.

    @paolomainardi paolomainardi changed the title feat: Handle the gitlab MR draft status feat: handle the gitlab MR draft status Aug 22, 2024
    @paolomainardi paolomainardi changed the title feat: handle the gitlab MR draft status feat: handle gitlab MR draft status Aug 22, 2024
    @paolomainardi
    Copy link
    Contributor Author

    /describe

    Copy link
    Contributor

    PR Description updated to latest commit (3778cc2)

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Aug 23, 2024

    thanks @paolomainardi

    @mrT23 mrT23 merged commit 4f1dccf into Codium-ai:main Aug 23, 2024
    2 checks passed
    @paolomainardi
    Copy link
    Contributor Author

    @mrT23 I am wondering if it is ok the scenario where we don't process comments when the MR is in the draft status; I see scenarios where you are developing and /improve could be very useful before changing it to the ready state.

    What do you think @mrT23 ? If I understood correctly, the GitHub app excludes just the MR open event when in the draft.

    @mrT23
    Copy link
    Collaborator

    mrT23 commented Aug 26, 2024

    people can always do online commenting

    we are talking here about auto comments

    @paolomainardi
    Copy link
    Contributor Author

    paolomainardi commented Aug 26, 2024

    This MR inhibits manual comments even when the MR is in draft.

    UPDATE: #1165 - With this, MR comments are processed again.

    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.

    Gitlab server should handle the draft status
    2 participants