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.

Sign up for GitHub

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
+11 −0
Merged

feat: handle gitlab MR draft status #1165

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ffaf5d5
feat: Handle the gitlab MR draft status
paolomainardi Aug 22, 2024
61837c6
Update gitlab_webhook.py
paolomainardi Aug 22, 2024
8793f8d
Update gitlab_webhook.py
paolomainardi Aug 22, 2024
3778cc2
feat: skip draft by default
paolomainardi Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pr_agent/servers/gitlab_webhook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,26 @@ async def inner(data: dict):
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')
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"}))

await _perform_commands_gitlab("pr_commands", PRAgent(), url, log_context)
elif data.get('object_kind') == 'note' and data.get('event_type') == 'note': # comment on MR
if 'merge_request' in data:
mr = data['merge_request']
url = mr.get('url')
draft = mr.get('draft')
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"}))

get_logger().info(f"A comment has been added to a merge request: {url}")
body = data.get('object_attributes', {}).get('note')
if data.get('object_attributes', {}).get('type') == 'DiffNote' and '/ask' in body: # /ask_line
Expand Down
1 change: 1 addition & 0 deletions pr_agent/settings/configuration.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.


[bitbucket_app]
pr_commands = [
Expand Down
Loading