Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2611803003:
Script to extract components from OWNERS files throughout the repo. (Closed)
DescriptionScript to extract components from OWNERS files throughout the repo.
This script traverses the src tree and parses OWNERS file for
COMPONENT/TEAM information. It gathers this information in the form of a
machine readable mapping and (optionally) saves it to disk.
R=stgao@chromium.org
BUG=667952
Review-Url: https://codereview.chromium.org/2611803003
Cr-Commit-Position: refs/heads/master@{#444938}
Committed: https://chromium.googlesource.com/chromium/src/+/e4f4643a0ddc25991785406d4a64b0e51d743ddc
Patch Set 1 #Patch Set 2 : Removing superfluous blank line #
Total comments: 14
Patch Set 3 : Adding tests, handling possible windows casing. #Patch Set 4 : Rebasing on top of landed change #
Total comments: 4
Patch Set 5 : Addressing feedback. #
Total comments: 2
Patch Set 6 : Adding first version of components_and_team_maps. #Patch Set 7 : Making robertocn and stgao owners of the generated file #Patch Set 8 : Simplifying check for existence of owners file. #Patch Set 9 : Shortening name of generated file. #
Total comments: 4
Patch Set 10 : Addressing feedback and extracting logic to shared module. #
Total comments: 21
Patch Set 11 : Addressing comments #
Total comments: 2
Patch Set 12 : Removing generated file, changing default output dir. #Patch Set 13 : No longer need these permissions #
Messages
Total messages: 32 (7 generated)
PTAL See dependent patchset for an example of the output of the script.
Description was changed from ========== Script to extract components from OWNERS files throughout the repo. This script traverses the src tree and parses OWNERS file for COMPONENT/TEAM information. It gathers this information in the form of a machine readable mapping and (optionally) saves it to disk. R=stgao@chromium.org BUG=667952 ========== to ========== Script to extract components from OWNERS files throughout the repo. This script traverses the src tree and parses OWNERS file for COMPONENT/TEAM information. It gathers this information in the form of a machine readable mapping and (optionally) saves it to disk. R=stgao@chromium.org BUG=667952 ==========
https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:6: """Parses OWNERS recursively and generates a machine readable component mapping. I think we have an assumption that the OWNERS file has valid TEAM/COMPONENT info here. It might be good to mention that explicitly. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:22: DEFAULT_SRC_LOCATION = os.path.join( nit: prefix it with a underscore as it is not shared outside this module. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:41: if 'TEAM:' in line: This seems to match something like: "# foo TEAM: bar". Will a regex help a little bit here? https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:74: component_to_team.setdefault(component, set()) How about using collection.defaultdict(set) ? https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:105: filename = os.path.join(DEFAULT_SRC_LOCATION, 'components.json') it also includes team info. Could we have a more inclusive file name? https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:107: f.write(data) Should we add some sort of header stating that the file is auto-generated and manual editing won't be preserved? For a json file, we could add a message field for this. There might be better options. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:146: results = json.dumps(unwrap(mappings), sort_keys=True, indent=4) Other json files seem to have indent=2 like https://cs.chromium.org/chromium/src/testing/buildbot/chromium.json https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:70: owners_file_name = [f for f in files if f.upper() == 'OWNERS'][0] This might cause exception if the directory has no OWNERS file?
https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:10: `# COMPONENT: Tools>Test>Findit` nit: The code of Findit is not in chromium. But it seems fine here.
PTAL, addressed all feedback. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:6: """Parses OWNERS recursively and generates a machine readable component mapping. On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > I think we have an assumption that the OWNERS file has valid TEAM/COMPONENT info > here. It might be good to mention that explicitly. Done. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:22: DEFAULT_SRC_LOCATION = os.path.join( On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > nit: prefix it with a underscore as it is not shared outside this module. Done. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:41: if 'TEAM:' in line: On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > This seems to match something like: "# foo TEAM: bar". > > Will a regex help a little bit here? Done. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:74: component_to_team.setdefault(component, set()) On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > How about using collection.defaultdict(set) ? Done. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:105: filename = os.path.join(DEFAULT_SRC_LOCATION, 'components.json') On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > it also includes team info. Could we have a more inclusive file name? Done. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:107: f.write(data) On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > Should we add some sort of header stating that the file is auto-generated and > manual editing won't be preserved? > > For a json file, we could add a message field for this. There might be better > options. Done. https://codereview.chromium.org/2611803003/diff/20001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:146: results = json.dumps(unwrap(mappings), sort_keys=True, indent=4) On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > Other json files seem to have indent=2 like > https://cs.chromium.org/chromium/src/testing/buildbot/chromium.json Done. https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:10: `# COMPONENT: Tools>Test>Findit` On 2017/01/05 17:22:30, stgao (slow on Monday) wrote: > nit: The code of Findit is not in chromium. But it seems fine here. Ack. It's just an example and a small shout-out to the team. https://codereview.chromium.org/2611803003/diff/60001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:70: owners_file_name = [f for f in files if f.upper() == 'OWNERS'][0] On 2017/01/05 07:56:52, stgao (slow on Monday) wrote: > This might cause exception if the directory has no OWNERS file? Yeah. Fixed.
Mind running the script with a up-to-date checkout and including the json file in this CL? We might want to update the src/OWNERS for this specific json file as well. https://codereview.chromium.org/2611803003/diff/80001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/80001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:82: if owners_file_name and owners_file_name in files: I thought `owners_file_name in files` is always true.
On 2017/01/05 19:48:36, stgao (slow on Monday) wrote: > Mind running the script with a up-to-date checkout and including the json file > in this CL? > We might want to update the src/OWNERS for this specific json file as well. > > https://codereview.chromium.org/2611803003/diff/80001/tools/checkteamtags/ext... > File tools/checkteamtags/extract_components.py (right): > > https://codereview.chromium.org/2611803003/diff/80001/tools/checkteamtags/ext... > tools/checkteamtags/extract_components.py:82: if owners_file_name and > owners_file_name in files: > I thought `owners_file_name in files` is always true. Done. PTAL
https://codereview.chromium.org/2611803003/diff/80001/tools/checkteamtags/ext... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/80001/tools/checkteamtags/ext... tools/checkteamtags/extract_components.py:82: if owners_file_name and owners_file_name in files: On 2017/01/05 19:48:36, stgao (slow on Monday) wrote: > I thought `owners_file_name in files` is always true. Done.
robertocn@chromium.org changed reviewers: + sshruthi@chromium.org
Shruthi, Could you review the format, contents and name of the json file generated by this script? I have added it this CL
On 2017/01/07 00:44:50, RobertoCN wrote: > Shruthi, > > Could you review the format, contents and name of the json file generated by > this script? I have added it this CL The json file format and contents LGTM. Thanks!
https://codereview.chromium.org/2611803003/diff/160001/OWNERS File OWNERS (right): https://codereview.chromium.org/2611803003/diff/160001/OWNERS#newcode13 OWNERS:13: per-file component_and_team_maps.json=robertocn@chromium.org This needs updating. https://codereview.chromium.org/2611803003/diff/160001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:97: def validate_mappings(m): Should we do the validation in the post-submit check instead? In that case, we might want the traverse/parse above to be in a separate module to be shared.
PTAL https://codereview.chromium.org/2611803003/diff/160001/OWNERS File OWNERS (right): https://codereview.chromium.org/2611803003/diff/160001/OWNERS#newcode13 OWNERS:13: per-file component_and_team_maps.json=robertocn@chromium.org On 2017/01/09 19:31:58, stgao (slow on Monday) wrote: > This needs updating. Done. https://codereview.chromium.org/2611803003/diff/160001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/160001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:97: def validate_mappings(m): On 2017/01/09 19:31:58, stgao (slow on Monday) wrote: > Should we do the validation in the post-submit check instead? > In that case, we might want the traverse/parse above to be in a separate module > to be shared. Done.
Some more comments. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:43: 'component_map.json') Fit in previous line. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:52: i.e. where src/ is expected to be. Mention about the default here? https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:67: 'mappings to instead of the default: src/components.json ' default file name was updated. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:88: print mapping_file_contents Do we still want to print the content after the errors? https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components_test.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components_test.py:125: self.assertIn('src has OWNERS but no COMPONENT', output) How about 'src/OWNERS has no COMPONENT' ? https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:12: """Search the file for lines that start with `# TEAM:` or `# COMPONENT:`. nit: Search -> Searches Same for others in following functions. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:35: def traverse(root): rename to be more specific about its functionality -- aggregate TEAM/COMPONENT info from OWNERS files? https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:43: {'component-to-team': {'Component1': 'team1@chr...', ...}, component is mapping to a set of team according to the code below. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:67: def validate_mappings(m): Rename it to be more specific like validate_one_team_for_one_component? https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:70: # TODO(robertocn): Validate the component names somehow. If we have a bug, maybe refer to that bug instead?
PTAL https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:43: 'component_map.json') On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > Fit in previous line. Done. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:52: i.e. where src/ is expected to be. On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > Mention about the default here? it says 'defaults to two levels up from this file's directory' in line 51. Is that sufficient? https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:67: 'mappings to instead of the default: src/components.json ' On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > default file name was updated. Done. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:88: print mapping_file_contents On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > Do we still want to print the content after the errors? I am adding an additional flag to force printing. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components_test.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components_test.py:125: self.assertIn('src has OWNERS but no COMPONENT', output) On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > How about 'src/OWNERS has no COMPONENT' ? Done. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:12: """Search the file for lines that start with `# TEAM:` or `# COMPONENT:`. On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > nit: Search -> Searches > > Same for others in following functions. Done. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:35: def traverse(root): On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > rename to be more specific about its functionality -- aggregate TEAM/COMPONENT > info from OWNERS files? decided to name it aggregate_components_from_owners. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:43: {'component-to-team': {'Component1': 'team1@chr...', ...}, On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > component is mapping to a set of team according to the code below. There is an unwrap call in the return statement that removes the set() wrapping. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:67: def validate_mappings(m): On 2017/01/10 06:19:11, stgao (slow on Monday) wrote: > Rename it to be more specific like validate_one_team_for_one_component? Done. https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:70: # TODO(robertocn): Validate the component names somehow. On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > If we have a bug, maybe refer to that bug instead? Done.
stgao@chromium.org changed reviewers: + jam@chromium.org
lgtm with name nits @jam, mind a review for adding the new file src/component_map.json? The background for this file is in this doc https://docs.google.com/document/d/1jty6UsFMW9-SYgpQC-ztEc3lltziOQBBArMkROCST... https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/180001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:52: i.e. where src/ is expected to be. On 2017/01/11 22:24:58, RobertoCN wrote: > On 2017/01/10 06:19:10, stgao (slow on Monday) wrote: > > Mention about the default here? > > it says 'defaults to two levels up from this file's directory' in line 51. Is > that sufficient? Acknowledged. https://codereview.chromium.org/2611803003/diff/200001/tools/checkteamtags/ex... File tools/checkteamtags/extract_components.py (right): https://codereview.chromium.org/2611803003/diff/200001/tools/checkteamtags/ex... tools/checkteamtags/extract_components.py:89: if options.force_print: name nit: force_print_mapping? https://codereview.chromium.org/2611803003/diff/200001/tools/checkteamtags/ow... File tools/checkteamtags/owners_file_tags.py (right): https://codereview.chromium.org/2611803003/diff/200001/tools/checkteamtags/ow... tools/checkteamtags/owners_file_tags.py:11: def parse(filename): name nit: extractTeamAndCompoentFromOwerFile or some other specific one?
On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > lgtm with name nits > > @jam, mind a review for adding the new file src/component_map.json? Why do we need to check in a file that is generated from other data? Will the users of this file have a checkout? If yes, they can run the script to get the data. If no, then they are pointing at a location somewhere on the web to get to the file. If so, maybe they can point at a buildbot step that ran this script?
On 2017/01/12 17:32:33, jam wrote: > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > > lgtm with name nits > > > > @jam, mind a review for adding the new file src/component_map.json? > > Why do we need to check in a file that is generated from other data? Good point. I think it just needs to be in a well-known location, not necessarily in src/ -- the most well-known one. > > Will the users of this file have a checkout? > If yes, they can run the script to get the data. > If no, then they are pointing at a location somewhere on the web to get to the > file. If so, maybe they can point at a buildbot step that ran this script? The user won't have a checkout, but we could put it in a Google Storage bucket instead. sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look good to you? Or you still prefer to make it in src/?
On Thu, Jan 12, 2017 at 3:28 PM <stgao@chromium.org> wrote: > On 2017/01/12 17:32:33, jam wrote: > > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > > > lgtm with name nits > > > > > > @jam, mind a review for adding the new file src/component_map.json? > > > > Why do we need to check in a file that is generated from other data? > > Good point. > I think it just needs to be in a well-known location, not necessarily in > src/ -- > the most well-known one. > > > > > > Will the users of this file have a checkout? > > If yes, they can run the script to get the data. > > If no, then they are pointing at a location somewhere on the web to get > to the > > file. If so, maybe they can point at a buildbot step that ran this > script? > > The user won't have a checkout, but we could put it in a Google Storage > bucket > instead. > > sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look good > to you? > Or you still prefer to make it in src/? > If the map/file exists somewhere (which I think it should), I would prefer that it is in some directory in src/, so that it is easy to find. It can be accessed by tools, which is the use case Shuotao mentioned, but can also serve as a quick reference for the complete mappings. Does that sound reasonable? > https://codereview.chromium.org/2611803003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/01/13 19:14:36, chromium-reviews wrote: > On Thu, Jan 12, 2017 at 3:28 PM <mailto:stgao@chromium.org> wrote: > > > On 2017/01/12 17:32:33, jam wrote: > > > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > > > > lgtm with name nits > > > > > > > > @jam, mind a review for adding the new file src/component_map.json? > > > > > > Why do we need to check in a file that is generated from other data? > > > > Good point. > > I think it just needs to be in a well-known location, not necessarily in > > src/ -- > > the most well-known one. > > > > > > > > > > Will the users of this file have a checkout? > > > If yes, they can run the script to get the data. > > > If no, then they are pointing at a location somewhere on the web to get > > to the > > > file. If so, maybe they can point at a buildbot step that ran this > > script? > > > > The user won't have a checkout, but we could put it in a Google Storage > > bucket > > instead. > > > > sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look good > > to you? > > Or you still prefer to make it in src/? > > > > If the map/file exists somewhere (which I think it should), I would prefer > that it is in some directory in src/, so that it is easy to find. It can be > accessed by tools, which is the use case Shuotao mentioned, Do you mean tools that are running in a checkout? Or tools that are run without a checkout? The former can just run the script. For the latter, can you give examples why this needs to be checked into chrome tree? Checking in the result of running a script is not usually something that we do. > but can also > serve as a quick reference for the complete mappings. Does that sound > reasonable? > > > > https://codereview.chromium.org/2611803003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
For the latter case, my reasoning was that if this has to live somewhere, it might be better for this to live on the tree somewhere, so it can also work as a quick reference of mappings. But, I do see your point that in those cases, it can just be generated by running the script, so I am fine with not checking in the result into the tree, but having it someplace else that tools can link to (that really is the primary use case that I am concerned with anyway). Shuotao/John, if we put this in a storage bucket somewhere like Shuotao suggested, can either of you think of any tools that WONT be able to access that in a clean manner? I would like the file to be a source of truth for any files that need this mapping and are not run on a checkout. On Fri, Jan 13, 2017 at 3:57 PM <jam@chromium.org> wrote: > On 2017/01/13 19:14:36, chromium-reviews wrote: > > > On Thu, Jan 12, 2017 at 3:28 PM <mailto:stgao@chromium.org> wrote: > > > > > On 2017/01/12 17:32:33, jam wrote: > > > > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > > > > > lgtm with name nits > > > > > > > > > > @jam, mind a review for adding the new file src/component_map.json? > > > > > > > > Why do we need to check in a file that is generated from other data? > > > > > > Good point. > > > I think it just needs to be in a well-known location, not necessarily > in > > > src/ -- > > > the most well-known one. > > > > > > > > > > > > > > Will the users of this file have a checkout? > > > > If yes, they can run the script to get the data. > > > > If no, then they are pointing at a location somewhere on the web to > get > > > to the > > > > file. If so, maybe they can point at a buildbot step that ran this > > > script? > > > > > > The user won't have a checkout, but we could put it in a Google Storage > > > bucket > > > instead. > > > > > > sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look > good > > > to you? > > > Or you still prefer to make it in src/? > > > > > > > If the map/file exists somewhere (which I think it should), I would > prefer > > that it is in some directory in src/, so that it is easy to find. It can > be > > accessed by tools, which is the use case Shuotao mentioned, > > Do you mean tools that are running in a checkout? Or tools that are run > without > a checkout? > > The former can just run the script. > > For the latter, can you give examples why this needs to be checked into > chrome > tree? Checking in the result of running a script is not usually something > that > we do. > > > > but can also > > serve as a quick reference for the complete mappings. Does that sound > > reasonable? > > > > > > > https://codereview.chromium.org/2611803003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2611803003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I can't think of a reason where it wouldn't be accessible, since it could just be served off a web server (i.e. per https://cloud.google.com/storage/docs/cloud-console#_sharingdata ) On Fri, Jan 13, 2017 at 4:05 PM, Shruthi Sreekanta <sshruthi@google.com> wrote: > For the latter case, my reasoning was that if this has to live somewhere, > it might be better for this to live on the tree somewhere, so it can also > work as a quick reference of mappings. But, I do see your point that in > those cases, it can just be generated by running the script, so I am fine > with not checking in the result into the tree, but having it someplace else > that tools can link to (that really is the primary use case that I am > concerned with anyway). Shuotao/John, if we put this in a storage bucket > somewhere like Shuotao suggested, can either of you think of any tools that > WONT be able to access that in a clean manner? I would like the file to be > a source of truth for any files that need this mapping and are not run on a > checkout. > > On Fri, Jan 13, 2017 at 3:57 PM <jam@chromium.org> wrote: > >> On 2017/01/13 19:14:36, chromium-reviews wrote: >> >> > On Thu, Jan 12, 2017 at 3:28 PM <mailto:stgao@chromium.org> wrote: >> > >> > > On 2017/01/12 17:32:33, jam wrote: >> > > > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: >> > > > > lgtm with name nits >> > > > > >> > > > > @jam, mind a review for adding the new file >> src/component_map.json? >> > > > >> > > > Why do we need to check in a file that is generated from other data? >> > > >> > > Good point. >> > > I think it just needs to be in a well-known location, not necessarily >> in >> > > src/ -- >> > > the most well-known one. >> > > >> > > >> > > > >> > > > Will the users of this file have a checkout? >> > > > If yes, they can run the script to get the data. >> > > > If no, then they are pointing at a location somewhere on the web to >> get >> > > to the >> > > > file. If so, maybe they can point at a buildbot step that ran this >> > > script? >> > > >> > > The user won't have a checkout, but we could put it in a Google >> Storage >> > > bucket >> > > instead. >> > > >> > > sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look >> good >> > > to you? >> > > Or you still prefer to make it in src/? >> > > >> > >> > If the map/file exists somewhere (which I think it should), I would >> prefer >> > that it is in some directory in src/, so that it is easy to find. It >> can be >> > accessed by tools, which is the use case Shuotao mentioned, >> >> Do you mean tools that are running in a checkout? Or tools that are run >> without >> a checkout? >> >> The former can just run the script. >> >> For the latter, can you give examples why this needs to be checked into >> chrome >> tree? Checking in the result of running a script is not usually something >> that >> we do. >> >> >> > but can also >> > serve as a quick reference for the complete mappings. Does that sound >> > reasonable? >> > >> > >> > > https://codereview.chromium.org/2611803003/ >> > > >> > >> > -- >> > You received this message because you are subscribed to the Google >> Groups >> > "Chromium-reviews" group. >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> >> https://codereview.chromium.org/2611803003/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
As John mentioned, we could make that GS storage directory/file accessible to the world, and it could be served directly from GS web server. robertocn@, mind making the needed change according this discussion? On 2017/01/14 01:01:43, jam wrote: > I can't think of a reason where it wouldn't be accessible, since it could > just be served off a web server (i.e. per > https://cloud.google.com/storage/docs/cloud-console#_sharingdata ) > > On Fri, Jan 13, 2017 at 4:05 PM, Shruthi Sreekanta <mailto:sshruthi@google.com> > wrote: > > > For the latter case, my reasoning was that if this has to live somewhere, > > it might be better for this to live on the tree somewhere, so it can also > > work as a quick reference of mappings. But, I do see your point that in > > those cases, it can just be generated by running the script, so I am fine > > with not checking in the result into the tree, but having it someplace else > > that tools can link to (that really is the primary use case that I am > > concerned with anyway). Shuotao/John, if we put this in a storage bucket > > somewhere like Shuotao suggested, can either of you think of any tools that > > WONT be able to access that in a clean manner? I would like the file to be > > a source of truth for any files that need this mapping and are not run on a > > checkout. > > > > On Fri, Jan 13, 2017 at 3:57 PM <mailto:jam@chromium.org> wrote: > > > >> On 2017/01/13 19:14:36, chromium-reviews wrote: > >> > >> > On Thu, Jan 12, 2017 at 3:28 PM <mailto:stgao@chromium.org> wrote: > >> > > >> > > On 2017/01/12 17:32:33, jam wrote: > >> > > > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > >> > > > > lgtm with name nits > >> > > > > > >> > > > > @jam, mind a review for adding the new file > >> src/component_map.json? > >> > > > > >> > > > Why do we need to check in a file that is generated from other data? > >> > > > >> > > Good point. > >> > > I think it just needs to be in a well-known location, not necessarily > >> in > >> > > src/ -- > >> > > the most well-known one. > >> > > > >> > > > >> > > > > >> > > > Will the users of this file have a checkout? > >> > > > If yes, they can run the script to get the data. > >> > > > If no, then they are pointing at a location somewhere on the web to > >> get > >> > > to the > >> > > > file. If so, maybe they can point at a buildbot step that ran this > >> > > script? > >> > > > >> > > The user won't have a checkout, but we could put it in a Google > >> Storage > >> > > bucket > >> > > instead. > >> > > > >> > > sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look > >> good > >> > > to you? > >> > > Or you still prefer to make it in src/? > >> > > > >> > > >> > If the map/file exists somewhere (which I think it should), I would > >> prefer > >> > that it is in some directory in src/, so that it is easy to find. It > >> can be > >> > accessed by tools, which is the use case Shuotao mentioned, > >> > >> Do you mean tools that are running in a checkout? Or tools that are run > >> without > >> a checkout? > >> > >> The former can just run the script. > >> > >> For the latter, can you give examples why this needs to be checked into > >> chrome > >> tree? Checking in the result of running a script is not usually something > >> that > >> we do. > >> > >> > >> > but can also > >> > serve as a quick reference for the complete mappings. Does that sound > >> > reasonable? > >> > > >> > > >> > > https://codereview.chromium.org/2611803003/ > >> > > > >> > > >> > -- > >> > You received this message because you are subscribed to the Google > >> Groups > >> > "Chromium-reviews" group. > >> > To unsubscribe from this group and stop receiving emails from it, send > >> an > >> email > >> > to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > >> > >> > >> https://codereview.chromium.org/2611803003/ > >> > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/01/14 02:03:02, stgao (slow on Monday) wrote: > As John mentioned, we could make that GS storage directory/file accessible to > the world, and it could be served directly from GS web server. > > robertocn@, mind making the needed change according this discussion? > > > On 2017/01/14 01:01:43, jam wrote: > > I can't think of a reason where it wouldn't be accessible, since it could > > just be served off a web server (i.e. per > > https://cloud.google.com/storage/docs/cloud-console#_sharingdata ) > > > > On Fri, Jan 13, 2017 at 4:05 PM, Shruthi Sreekanta > <mailto:sshruthi@google.com> > > wrote: > > > > > For the latter case, my reasoning was that if this has to live somewhere, > > > it might be better for this to live on the tree somewhere, so it can also > > > work as a quick reference of mappings. But, I do see your point that in > > > those cases, it can just be generated by running the script, so I am fine > > > with not checking in the result into the tree, but having it someplace else > > > that tools can link to (that really is the primary use case that I am > > > concerned with anyway). Shuotao/John, if we put this in a storage bucket > > > somewhere like Shuotao suggested, can either of you think of any tools that > > > WONT be able to access that in a clean manner? I would like the file to be > > > a source of truth for any files that need this mapping and are not run on a > > > checkout. > > > > > > On Fri, Jan 13, 2017 at 3:57 PM <mailto:jam@chromium.org> wrote: > > > > > >> On 2017/01/13 19:14:36, chromium-reviews wrote: > > >> > > >> > On Thu, Jan 12, 2017 at 3:28 PM <mailto:stgao@chromium.org> wrote: > > >> > > > >> > > On 2017/01/12 17:32:33, jam wrote: > > >> > > > On 2017/01/12 02:45:32, stgao (slow on Monday) wrote: > > >> > > > > lgtm with name nits > > >> > > > > > > >> > > > > @jam, mind a review for adding the new file > > >> src/component_map.json? > > >> > > > > > >> > > > Why do we need to check in a file that is generated from other data? > > >> > > > > >> > > Good point. > > >> > > I think it just needs to be in a well-known location, not necessarily > > >> in > > >> > > src/ -- > > >> > > the most well-known one. > > >> > > > > >> > > > > >> > > > > > >> > > > Will the users of this file have a checkout? > > >> > > > If yes, they can run the script to get the data. > > >> > > > If no, then they are pointing at a location somewhere on the web to > > >> get > > >> > > to the > > >> > > > file. If so, maybe they can point at a buildbot step that ran this > > >> > > script? > > >> > > > > >> > > The user won't have a checkout, but we could put it in a Google > > >> Storage > > >> > > bucket > > >> > > instead. > > >> > > > > >> > > sshruthi@, will gs://findit-for-me.appspot.com/component_maps/ look > > >> good > > >> > > to you? > > >> > > Or you still prefer to make it in src/? > > >> > > > > >> > > > >> > If the map/file exists somewhere (which I think it should), I would > > >> prefer > > >> > that it is in some directory in src/, so that it is easy to find. It > > >> can be > > >> > accessed by tools, which is the use case Shuotao mentioned, > > >> > > >> Do you mean tools that are running in a checkout? Or tools that are run > > >> without > > >> a checkout? > > >> > > >> The former can just run the script. > > >> > > >> For the latter, can you give examples why this needs to be checked into > > >> chrome > > >> tree? Checking in the result of running a script is not usually something > > >> that > > >> we do. > > >> > > >> > > >> > but can also > > >> > serve as a quick reference for the complete mappings. Does that sound > > >> > reasonable? > > >> > > > >> > > > >> > > https://codereview.chromium.org/2611803003/ > > >> > > > > >> > > > >> > -- > > >> > You received this message because you are subscribed to the Google > > >> Groups > > >> > "Chromium-reviews" group. > > >> > To unsubscribe from this group and stop receiving emails from it, send > > >> an > > >> email > > >> > to mailto:chromium-reviews+unsubscribe@chromium.org. > > >> > > >> > > >> > > >> https://codereview.chromium.org/2611803003/ > > >> > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. I will be finishing this today. Do people like the idea of having the file in this url? https://storage.googleapis.com/chromium-owners/component_map.json
> > I will be finishing this today. > > Do people like the idea of having the file in this url? > https://storage.googleapis.com/chromium-owners/component_map.json This looks good to me.
The CQ bit was checked by robertocn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sshruthi@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2611803003/#ps240001 (title: "No longer need these permissions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1484870330254980,
"parent_rev": "d8f63bf523b712731fe2481c997b6f98894d12bb", "commit_rev":
"e4f4643a0ddc25991785406d4a64b0e51d743ddc"}
Message was sent while issue was closed.
Description was changed from ========== Script to extract components from OWNERS files throughout the repo. This script traverses the src tree and parses OWNERS file for COMPONENT/TEAM information. It gathers this information in the form of a machine readable mapping and (optionally) saves it to disk. R=stgao@chromium.org BUG=667952 ========== to ========== Script to extract components from OWNERS files throughout the repo. This script traverses the src tree and parses OWNERS file for COMPONENT/TEAM information. It gathers this information in the form of a machine readable mapping and (optionally) saves it to disk. R=stgao@chromium.org BUG=667952 Review-Url: https://codereview.chromium.org/2611803003 Cr-Commit-Position: refs/heads/master@{#444938} Committed: https://chromium.googlesource.com/chromium/src/+/e4f4643a0ddc25991785406d4a64... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/e4f4643a0ddc25991785406d4a64... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
