Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

art-2519 add check upstreampulls function #488

Merged
merged 11 commits into from
Nov 2, 2021
55 changes: 54 additions & 1 deletion doozerlib/cli/images_streams.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from doozerlib.model import Missing
from doozerlib.pushd import Dir
from doozerlib.cli import cli, pass_runtime
from doozerlib import brew, state, exectools, model
from doozerlib import brew, state, exectools, model, constants
from doozerlib.util import get_docker_config_json, convert_remote_git_to_ssh, \
split_git_url, remove_prefix, green_print,\
yellow_print, red_print, convert_remote_git_to_https, \
Expand Down Expand Up @@ -530,6 +530,59 @@ def prs():
pass


@prs.command('list', short_help='List all open prs for upstream repo')
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the description here should note that the token env var is required

@pass_runtime
def images_upstreampulls(runtime):
runtime.initialize(clone_distgits=False, clone_source=False)
retdata = {}
upstreams = []
Copy link
Contributor

Choose a reason for hiding this comment

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

efficiency isn't exactly important here, but just on principle i would not make this a list since order doesn't matter...

Suggested change
upstreams = []
upstreams = set()

github_client = Github(constants.GTIHUB_TOKEN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
github_client = Github(constants.GTIHUB_TOKEN)
github_client = Github(constants.GITHUB_TOKEN)

also I think you want to pass the contents of the env var, not its name:

Suggested change
github_client = Github(constants.GTIHUB_TOKEN)
github_client = Github(os.environ.get(constants.GITHUB_TOKEN))

for image_meta in runtime.ordered_image_metas():
time.sleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

... why? with a proper github token the rate limiting should not kick in

source_repo_url, source_repo_branch = _get_upstream_source(runtime, image_meta, skip_branch_check=True)
if not source_repo_url or 'github.com' not in source_repo_url:
runtime.logger.info('Skipping PR check since there is no configured github source URL')
continue

public_repo_url, public_branch = runtime.get_public_upstream(source_repo_url)
if not public_branch:
public_branch = source_repo_branch
_, org, repo_name = split_git_url(public_repo_url)
if public_repo_url in upstreams:
continue
else:
upstreams.append(public_repo_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for else
and if you follow my suggestion and make it a set then it would be

Suggested change
else:
upstreams.append(public_repo_url)
upstreams.add(public_repo_url)

public_source_repo = github_client.get_repo(f'{org}/{repo_name}')
pulls = public_source_repo.get_pulls(state='open', sort='created')
for pr in pulls:
time.sleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

should go a lot faster without this

Suggested change
time.sleep(1)

if pr.user.login == github_client.get_user().login and pr.base.ref == source_repo_branch:
if pr.assignee is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

so... PRs with no assignee are just ignored?
e.g. openshift/kubernetes#934
I don't think the approach of looking at assignees is actually what we want. i know the original card says to nag people by slack, but i'm not sure we can accomplish that since we don't have a mapping of slack handles. what we can easily do is look up the owners email(s) in the metadata entries. i think that is the place to start.

if pr.assignee.email is not None:
if pr.assignee.email not in retdata.keys():
retdata[pr.assignee.email] = {}
if public_repo_url not in retdata[pr.assignee.email].keys():
retdata[pr.assignee.email][public_repo_url] = []
retdata[pr.assignee.email][public_repo_url].append("[{} ][created_at:{}]".format(
pr.html_url, pr.created_at))
else:
if "no assignee" not in retdata.keys():
retdata["no assignee"] = {}
if public_repo_url not in retdata["no assignee"].keys():
retdata["no assignee"][public_repo_url] = []
retdata["no assignee"][public_repo_url].append("[{} ][created_at:{}]".format(
pr.html_url, pr.created_at))
# format output
for key, val in retdata.items():
if len(val) == 0:
continue
print(">[{}]".format(key))
for img, prs in val.items():
print(" -[{}]".format(img))
for pr in prs:
print(" {}".format(pr))
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting yaml yourself is a recipe for disaster, how about just using the yaml module...



@prs.command('open', short_help='Open PRs against upstream component repos that have a FROM that differs from ART metadata.')
@click.option('--github-access-token', metavar='TOKEN', required=True, help='Github access token for user.')
@click.option('--bug', metavar='BZ#', required=False, default=None, help='Title with Bug #: prefix')
Expand Down
2 changes: 2 additions & 0 deletions doozerlib/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
"GIT_SSH_COMMAND": "ssh -oBatchMode=yes",
"GIT_TERMINAL_PROMPT": "0",
}

GTIHUB_TOKEN = "GITHUB_TOKEN"
sosiouxme marked this conversation as resolved.
Show resolved Hide resolved
BREWWEB_URL = "https://brewweb.engineering.redhat.com/brew"

# Environment variables that should be set for doozer interaction with db for storing and retrieving build records.
Expand Down