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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vfreex
suggested changes
Aug 12, 2021
| from doozerlib import constants | ||
| from doozerlib.cli import cli, pass_runtime | ||
|
|
||
| github_token = os.getenv(constants.GIT_TOKEN) |
There was a problem hiding this comment.
Git != GitHub. There is no such thing called GIT_TOKEN. Would like to rename it to something like GITHUB_TOKEN.
| repo = github_client.get_repo("openshift/ocp-build-data") | ||
|
|
||
|
|
||
| @cli.command("images:upstreampulls", short_help="Check upstream PRs in open status") |
There was a problem hiding this comment.
It doesn't seem to me that the verb name images:upstreampulls or the help message explains well about what it intends to do. Would like to have a better verb name like images:streams prs list for consistency and improved help message.
|
|
||
| github_token = os.getenv(constants.GIT_TOKEN) | ||
| github_client = Github(github_token) | ||
| repo = github_client.get_repo("openshift/ocp-build-data") |
There was a problem hiding this comment.
openshift/ocp-build-data shouldn't be hardcoded here. How could I run the doozer with my own fork of ocp-build-data?
Global variables should be avoided as long as you can.
Comment on lines
21
to
35
| version = runtime.group.split("-")[-1] | ||
| retdata = {} | ||
| contents = repo.get_contents("images", ref="openshift-" + version) | ||
| for content_file in contents: | ||
| ydata = yaml.load(content_file.decoded_content, Loader=yaml.FullLoader) | ||
| if 'content' not in ydata: | ||
| continue | ||
| if 'git' not in ydata['content']['source']: | ||
| continue | ||
| upstream_project = ydata['content']['source']['git']['url'].split(':')[-1].split('/')[0] | ||
| upstream = ydata['content']['source']['git']['url'].split('/')[-1].split('.')[0] | ||
| if upstream_project == "openshift-priv": | ||
| xrepo = github_client.get_repo("openshift/" + upstream) | ||
| else: | ||
| xrepo = github_client.get_repo(upstream_project + "/" + upstream) |
There was a problem hiding this comment.
See https://github.com/openshift/doozer/blob/c7e31761b3ec312d4c3101bd3c93c94fc53c5025/doozerlib/cli/images_streams.py#L650 for how to correctly resolve the upstream repo url and branch for each image.
| xrepo = github_client.get_repo(upstream_project + "/" + upstream) | ||
| pulls = xrepo.get_pulls(state='open', sort='created') | ||
| for pr in pulls: | ||
| if pr.user.login == "openshift-bot" and pr.base.ref == "release-" + version: |
There was a problem hiding this comment.
Please don't hardcode the login name and branch name.
login should be configurable, e.g. via command line option, and the branch name is not always follow release-*. There are exceptions.
Comment on lines
55
to
62
| 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)) |
sosiouxme
suggested changes
Oct 18, 2021
doozerlib/cli/images_streams.py
Outdated
| upstreams = [] | ||
| github_client = Github(constants.GTIHUB_TOKEN) | ||
| for image_meta in runtime.ordered_image_metas(): | ||
| time.sleep(1) |
There was a problem hiding this comment.
... why? with a proper github token the rate limiting should not kick in
doozerlib/cli/images_streams.py
Outdated
| runtime.initialize(clone_distgits=False, clone_source=False) | ||
| retdata = {} | ||
| upstreams = [] | ||
| github_client = Github(constants.GTIHUB_TOKEN) |
There was a problem hiding this comment.
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)) |
doozerlib/cli/images_streams.py
Outdated
| def images_upstreampulls(runtime): | ||
| runtime.initialize(clone_distgits=False, clone_source=False) | ||
| retdata = {} | ||
| upstreams = [] |
There was a problem hiding this comment.
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() |
doozerlib/cli/images_streams.py
Outdated
Comment on lines
553
to
554
| else: | ||
| upstreams.append(public_repo_url) |
There was a problem hiding this comment.
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) |
doozerlib/cli/images_streams.py
Outdated
| 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) |
There was a problem hiding this comment.
should go a lot faster without this
Suggested change
| time.sleep(1) |
doozerlib/cli/images_streams.py
Outdated
| for pr in pulls: | ||
| 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: |
There was a problem hiding this comment.
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.
doozerlib/cli/images_streams.py
Outdated
Comment on lines
576
to
583
| 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)) |
There was a problem hiding this comment.
formatting yaml yourself is a recipe for disaster, how about just using the yaml module...
sosiouxme
reviewed
Oct 28, 2021
doozerlib/cli/images_streams.py
Outdated
| @@ -530,6 +532,40 @@ def prs(): | |||
| pass | |||
|
|
|||
|
|
|||
| @prs.command('list', short_help='List all open prs for upstream repo') | |||
There was a problem hiding this comment.
i think the description here should note that the token env var is required
doozerlib/cli/images_streams.py
Outdated
Comment on lines
560
to
565
| if owners_email not in retdata.keys(): | ||
| retdata[owners_email] = {} | ||
| if public_repo_url not in retdata[owners_email].keys(): | ||
| retdata[owners_email][public_repo_url] = [] | ||
| retdata[owners_email][public_repo_url].append("[{} ][created_at:{}]".format( | ||
| pr.html_url, pr.created_at)) |
There was a problem hiding this comment.
a bit more concise, and also frankly i don't see a reason to format this data at all:
Suggested change
| if owners_email not in retdata.keys(): | |
| retdata[owners_email] = {} | |
| if public_repo_url not in retdata[owners_email].keys(): | |
| retdata[owners_email][public_repo_url] = [] | |
| retdata[owners_email][public_repo_url].append("[{} ][created_at:{}]".format( | |
| pr.html_url, pr.created_at)) | |
| retdata.setdefault(owners_email, {}).setdefault(public_repo_url, []).append( | |
| dict(pr_url=pr.html_url, created_at=pr.created_at) | |
| ) |
sosiouxme
reviewed
Oct 28, 2021
doozerlib/cli/images_streams.py
Outdated
| pulls = public_source_repo.get_pulls(state='open', sort='created') | ||
| for pr in pulls: | ||
| if pr.user.login == github_client.get_user().login and pr.base.ref == source_repo_branch: | ||
| owners_email = image_meta.config['owners'][0] |
There was a problem hiding this comment.
why only the first? why not spam them all?
Suggested change
| owners_email = image_meta.config['owners'][0] | |
| for owners_email in image_meta.config['owners']: |
Build #9 |
|
sosiouxme
approved these changes
Nov 2, 2021
vfreex
added a commit
to vfreex/doozer
that referenced
this pull request
Nov 19, 2021
e810dcb Permit assemblies to use custom rhcos builds The RHCOS pipeline, when configured for a custom build, will bucket the resulting metadata files in s3 in a directory suffixed with `-custom`. In order to for assemblies to use custom RHCOS builds, we must try to find metadata in either the standard location or `-custom` location. 0a4d5ca [ART-3109] Use container.yaml to apply floating tags (openshift-eng#524) e3e6315 ART-3437: Make bundle tag names configurable 36b3291 Prevent leading zero in semver (openshift-eng#527) 57af5c8 Revert "make bundle tag names configurable (openshift-eng#532)" (openshift-eng#533) 3a14f91 make bundle tag names configurable (openshift-eng#532) 3c16d82 Allow olm-bundle:print to skip when builds are missing b3cc4c3 We are in the future 69e613e Update code to work with flexmock 0.11 4107d82 ART-2519 add check upstreampulls function (openshift-eng#488) 56325c6 bump-doozer: add annotated tag
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add check imagestreams prs list function in doozer used by image-health check job to collect all opened pr of a target release.