Add ddev ci codeowners command
#21312
Open
+219
−6
Conversation
This file contains hidden or 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
|
This PR does not modify any files shipped with the agent. To help streamline the release process, please consider adding the |
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
NouemanKHAL
reviewed
Sep 10, 2025
NouemanKHAL
reviewed
Sep 17, 2025
Comment on lines
+3
to
+8
| Options: | ||
|
|
||
| - `--pr <number>`: Show codeowners for files changed in a pull request. | ||
| - `--sha <commit>`: Show codeowners for files changed in a commit. | ||
| - `--files <list>`: Show codeowners for a comma-separated list of files. | ||
| - `--per-file`: Output owners per file instead of a combined list. |
There was a problem hiding this comment.
Suggested change
| Options: | |
| - `--pr <number>`: Show codeowners for files changed in a pull request. | |
| - `--sha <commit>`: Show codeowners for files changed in a commit. | |
| - `--files <list>`: Show codeowners for a comma-separated list of files. | |
| - `--per-file`: Output owners per file instead of a combined list. | |
| Options: | |
| - `--pr <number>`: Show codeowners for files changed in a pull request. | |
| - `--sha <commit>`: Show codeowners for files changed in a commit. | |
| - `--files <list>`: Show codeowners for a comma-separated list of files. | |
| - `--per-file`: Output owners per file instead of a combined list. |
IMO this is maybe too much details for a changelog entry, we can remove this.
| @click.option('--files', help='Check the codeowners for the given files separated by commas') | ||
| @click.option('--per-file', is_flag=True, help='Output the codeowners for each file') | ||
| @click.pass_obj | ||
| def codeowners(app: Application, pr: str, sha: str, files: str, per_file: bool): |
There was a problem hiding this comment.
Suggested change
| def codeowners(app: Application, pr: str, sha: str, files: str, per_file: bool): | |
| def codeowners(app: Application, pr: str, commit_sha: str, files: str, per_file: bool): |
Comment on lines
+17
to
+20
| codeowners_file = app.repo.path / '.github' / 'CODEOWNERS' | ||
| with open(codeowners_file, 'r') as f: | ||
| codeowners_content = f.readlines() | ||
| codeowners: CodeOwners = CodeOwners("\n".join(codeowners_content)) |
There was a problem hiding this comment.
Suggested change
| codeowners_file = app.repo.path / '.github' / 'CODEOWNERS' | |
| with open(codeowners_file, 'r') as f: | |
| codeowners_content = f.readlines() | |
| codeowners: CodeOwners = CodeOwners("\n".join(codeowners_content)) | |
| codeowners_file = app.repo.path / ".github" / "CODEOWNERS" | |
| if not codeowners_file.exists(): | |
| raise FileNotFoundError(f"{codeowners_file} does not exist") | |
| codeowners_content = codeowners_file.read_text() | |
| codeowners = CodeOwners(codeowners_content) |
suggestion:
- Check for codeowners file existence and raise an exception if it doesn't exist.
- Remove unnecessary type hint in the
codeownersvariable, the CodeOwner type/class is already shown in the value. - Using the Path.read_text method opens the file, reads it in text-mode and returns its contents making sure it gets closed, using this method is simpler and removes the need for manually using a context manager
with.
Comment on lines
+22
to
+40
| if files: | ||
| file_list = files.split(',') if ',' in files else [files] | ||
| elif pr: | ||
| github = app.github | ||
| pr_obj = github.get_pull_request_by_number(pr) | ||
| if pr_obj is None: | ||
| app.display_error(f'Pull request {pr} not found') | ||
| return | ||
| file_list = github.get_changed_files_by_pr(pr_obj) | ||
| elif sha: | ||
| github = app.github | ||
| files_by_sha = github.get_changed_files_by_sha(sha) | ||
| if files_by_sha is None: | ||
| app.display_error(f'Commit {sha} not found') | ||
| return | ||
| file_list = files_by_sha | ||
| else: | ||
| app.display_error('No files provided') | ||
| return |
There was a problem hiding this comment.
So we have a precedence over these flags, we first check for files then pr then the commit sha etc, is this behavior documented in the command help?
I think we should add a log that shows which flag is being used.
Comment on lines
+55
to
+58
| if owners: | ||
| # Collect all owner names for this file in a list | ||
| owner_names = [owner_name for _, owner_name in owners] | ||
| file_to_owners[file] = sorted(owner_names) |
There was a problem hiding this comment.
What should we do in the case where a file doesn't have an owner? I believe that it could be good to have the option to display all files that have no owner as well in a certain way.
Comment on lines
+41
to
+46
| if per_file: | ||
| owners_per_file = map_files_to_teams(file_list, codeowners) | ||
| app.display_info(owners_per_file) | ||
| else: | ||
| owners = get_owners(file_list, codeowners) | ||
| app.display_info(sorted(owners)) |
There was a problem hiding this comment.
Suggested change
| if per_file: | |
| owners_per_file = map_files_to_teams(file_list, codeowners) | |
| app.display_info(owners_per_file) | |
| else: | |
| owners = get_owners(file_list, codeowners) | |
| app.display_info(sorted(owners)) | |
| if per_file: | |
| owners_per_file = get_owners_by_file(file_list, codeowners) | |
| app.display_info(owners_per_file) | |
| else: | |
| owners = get_all_owners(file_list, codeowners) | |
| app.display_info(sorted(owners)) |
suggestion: in order to stay consistent with the get_owners function name, I think it's better to rename the map_files_to_teams function to follow the same naming, since both functions are doing similar kind of work.
|
|
||
| @click.command() | ||
| @click.option('--pr', help='Check the codeowners for the given PR') | ||
| @click.option('--sha', help='Check the codeowners for the given sha') |
There was a problem hiding this comment.
Suggested change
| @click.option('--sha', help='Check the codeowners for the given sha') | |
| @click.option('--sha', help='Check the codeowners for the given commit SHA') |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
Adds a new ddev command
ddev ci codeownersto check repository CODEOWNERS for pull requests, commits, or specific files.Options:
--pr <number>: Show codeowners for files changed in a pull request.--sha <commit>: Show codeowners for files changed in a commit.--files <list>: Show codeowners for a comma-separated list of files.--per-file: Output owners per file instead of a combined list.Motivation
The size quality gates will enforce policies around newly added dependencies. To do this, they must identify which teams introduced those changes in a PR.
Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged