feat: move generate_code_owner_mappings and add enterprise override #321
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
rgraber
force-pushed
the
rsgraber/20230605-multiple-enterprise-owners
branch
from
67e33b2
to
12595a0
Compare
rgraber
force-pushed
the
rsgraber/20230605-multiple-enterprise-owners
branch
from
a277d30
to
7a6c8a1
Compare
rgraber
force-pushed
the
rsgraber/20230605-multiple-enterprise-owners
branch
from
51becd4
to
68f4203
Compare
robrap
reviewed
Jun 5, 2023
| @@ -166,7 +166,11 @@ def _map_repo_apps(csv_type, repo_csv, app_to_repo_map, owner_map, owner_to_path | |||
| csv_repo_to_owner_map[row.get('repo url')] = owner | |||
|
|
|||
| for app, repo_url in app_to_repo_map.items(): | |||
| owner = csv_repo_to_owner_map.get(repo_url, None) | |||
| # special case: enterprise-quokkas owns integrated channels but not the rest of edx-enterprise | |||
| if app == "integrated_channels": | |||
There was a problem hiding this comment.
If you are going to hard code, could we at least add EDX_REPO_APPS_OVERRIDES or something like that so all the configs are together and this isn't buried in the code? The data structure would make it simpler to add others, but that is not the reason. It is difficult to figure out where this mapping code happens, and burying in the code itself just makes it that much harder.
robrap
approved these changes
Jun 5, 2023
…ngs.py Co-authored-by: Robert Raposa <[email protected]>
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.
Copies generate_code_owner_mappings and adds a hardcoded exception where enterprise-quokkas owns edx-enterprise/integrated_paths. Somewhat hacky but it seems likely this is the only override we need so it makes sense to put it in the script rather than reformat all the data.
TODO after:
Best viewed as separate commits.
The first commit just copied the script from edx-platform
The second commit added the override
The third commit is just some additional comments around things I found confusing
Merge checklist:
Check off if complete or not applicable: