PR#20: Checking if the branch requestors are part of maintainers or not - fedscm-admin

fedscm-admin

#20 Checking if the branch requestors are part of maintainers or not

Merged 4 years ago by mohanboddu. Opened 4 years ago by mohanboddu.

mohanboddu/fedscm-admin maintainer-check into master

Checking if the branch requestors are part of maintainers or not

Mohan Boddu • 4 years ago

cded820

fedscm_admin/utils.py

file modified

+16 -2

		`@@ -270,7 +270,8 @@`
		`initial_commit=issue_body.get(`
		`'initial_commit', True))`
		`elif issue_body.get('action') == 'new_branch':`
		`- prompt_for_new_branch(issue, issue_body, auto_approve=auto_approve)`
		`+ prompt_for_new_branch(issue, issue_body, force=force,`
		`+ auto_approve=auto_approve)`
		`else:`
		`prompt_to_close_bad_ticket(issue, "Invalid or missing action field")`
		`return`
		`@@ -480,7 +481,7 @@`
		`return`


		`- def prompt_for_new_branch(issue_json, issue_body_json, auto_approve=False):`
		`+ def prompt_for_new_branch(issue_json, issue_body_json, force=False, auto_approve=False):`
		`"""`
		`A helper function that prompts the user with information on a new branch`
		`ticket`
		`@@ -549,6 +550,19 @@`
		`issue_owner = issue_json['user']['name']`
		`issue_ui_url = fedscm_admin.pagure.get_pagure_issue_url(issue_id)`

		`+ # Check if the branch requestor is one of the maintainers`
		`+ click.echo('- Checking if {0} is one of the maintainers of the package'.format(issue_owner))`
		`+ if force:`
		`+ msg = ' WARNING: Checking of maintainers is skipped'`
		`+ click.secho(msg, fg='yellow')`
		`+ else:`
		`+ maintainers = set(project['access_users']['owner']) \| set(project['access_users']['admin'])`
		`+ if issue_owner not in maintainers:`
		`+ prompt_to_close_bad_ticket(`
		`+ issue_json, '{0} is not a maintainer of the {1} package'.format(issue_owner, repo)`
		`+ )`
		`+ return`
		`+`
		`if auto_approve and \`
		`not ticket_requires_approval('new_branch', issue_body_json):`
		`click.echo('- Auto-approving the new branch request for "{0}" on '`

mohanboddu commented 4 years ago

Signed-off-by: Mohan Boddu mboddu@bhujji.com

mohanboddu commented 4 years ago

Fix for https://pagure.io/releng/issue/8844

mprahl commented 4 years ago

This looks good to me. It'd be nice to have a unit test for it though.

pghmcfc commented 4 years ago

Does this allow branch requests by provenpackagers? Should it?

kevin commented 4 years ago

It shouldn't.

ignatenkobrain commented 4 years ago

lgtm

mohanboddu commented 4 years ago

It shouldn't.

@kevin Do you think we should add that? If we should add it, how can I access the list? FAS?

kevin commented 4 years ago

Well, there has to be a way to handle:

When a branch is urgently needed to fis some issue (perhaps release blocking?) If releng has a way to override this would work in that case.

When someone requests a branch and the maintainer hasn't reploied in X amount of time (I think we used to give them 2 weeks in the past). I'm not sure how to handle this case. We could leave the ticket until it's 2 weeks old or wharever, but then we don't really know if the maintainer answered and said no, don't do it, or not.

So, perhaps this entire workflow needs more discussion?

mohanboddu commented 4 years ago

Well, there has to be a way to handle:

When a branch is urgently needed to fis some issue (perhaps release blocking?) If releng has a way to override this would work in that case.

This part is already handled in the patch, you can force to skip the maintainers check.

or

When someone requests a branch and the maintainer hasn't reploied in X amount of time (I think we used to give them 2 weeks in the past). I'm not sure how to handle this case. We could leave the ticket until it's 2 weeks old or wharever, but then we don't really know if the maintainer answered and said no, don't do it, or not.

So, perhaps this entire workflow needs more discussion?

Yeah, this requires more discussion.