feat: add node mono repo post processor #1471
Conversation
sofisl
requested review from
SurferJeffAtGoogle,
chingor13,
bcoe and
a team
as code owners
There was a problem hiding this comment.
Left some small refactoring suggestions. Mainly, I believe there are a few places where you could just import a method from node.py, rather than copying the helpers.
Algorithmically, I think there's a risk that glob won't scale well as we add 1000s of files. I would instead consider a simple directory list, since we know that files will be in a well known location.
Overall, I think this is great work 🎉 , it's great that you got through tests in place.
| @@ -326,6 +326,36 @@ def node_library(self, **kwargs) -> Path: | |||
|
|
|||
| return self._generic_library("node_library", **kwargs) | |||
|
|
|||
| def node_mono_repo_library(self, relative_dir, **kwargs) -> Path: | |||
There was a problem hiding this comment.
since this logic is pretty much identical to def node_library, I might just make node_library take an optional relative_dir parameter, and then default to ./ unless there's an alternate relative_dir provided.
There was a problem hiding this comment.
Normally, I think it would be better to simply modify the existing code rather than duplicating it. However, I think in this case it's better to duplicate it, because: 1) it makes it easier to read and debug (just follow the node_mono_repo naming), and 2) we are eventually migrating to a mono repo, and eventually all the split repo paths will be deprecated. Given that it's only a few helper methods that are truly being copied over, I'd opt for copying over adding a new path to a helper method (this goes for other helper methods that are similar as well). Furthermore, if there are more specific changes we need to make for mono-repos, it will be easier to add to a mono-repo only path rather than making sure it doesn't affect split repos.
| data["repository"] = f'{repo["owner"]}/{repo["name"]}' | ||
| data["repository_name"] = repo["name"] | ||
| data["lib_install_cmd"] = f'npm install {data["name"]}' | ||
| engines_field = re.search(r"([0-9][0-9])", data["engines"]["node"]) |
There was a problem hiding this comment.
I would switch this to [0-9]+, prevents a bug if Node ever goes to weekly releases like Chrome (which is up to version 101).
There was a problem hiding this comment.
If this logic is branched from the node module, I'd be ok leaving it and following up with a fix to both rather than diverging here.
Also if you want to keep the minimum of 2 digits \d{2,} could work (\d+ is one or more)
There was a problem hiding this comment.
I will do in a follow-up fix - I only added line 54-55 because the linter was complaining.
| Returns: | ||
| The name of the key to fetch the publish token. | ||
| """ | ||
| return package_name.strip("@").replace("/", "-") + "-npm-token" |
There was a problem hiding this comment.
We use a single token for all publication now, so this helper is obsolete, i.e., we should no longer be referencing publish_token anywhere.
| return re.findall(r"\{(.*Client)\}", content) | ||
|
|
||
|
|
||
| def generate_index_ts( |
There was a problem hiding this comment.
I think several of these helpers are identical to languages/node.py?
I think you could import the helpers from node.py, so that we don't have duplicate code (you can import individual functions).
| A list of all metadata files. | ||
| """ | ||
| owlbot_dirs = [] | ||
| for path_object in dir.glob("**/*"): |
There was a problem hiding this comment.
I think there's a risk that this glob will get slow once we have dozens of repositories in the repo, as it's going to just create a list of all files in the entire mono-repo.
By convention, we're always going to have the .OwlBot.yaml in one location:
packages/
a/.OwlBot.yaml
b/.OwlBot.yaml
b/.OwlBot.yaml
Given this, I would just use os.listdir, to list directories in packages/, and then check for a .OwlBot.yaml in each folder.
There was a problem hiding this comment.
Another option too would be to try using the glob **/.OwlBot.yaml. The work is all on the glob side and you won't iterate over unnecessary files.
Same as #1470, with easier-to-read diff