Have way to disable Javascript Package Ownership
cszatmary opened this issue · 6 comments
First off, thanks for making this gem! We have been searching for a way to assign and manage ownership of files within our rails monolith and this looks like a great solution.
While setting it up, I ran into an issue involving Javascript Package Ownership which lead to confusion about how this feature interacts with unowned_globs.
Background
I ran bin/codeownership validate which produced this error:
/Users/chrisszatmary/.rvm/gems/ruby-3.2.2/gems/code_ownership-1.34.2/lib/code_ownership/private/parse_js_packages.rb:44:in `rescue in from': #<JSON::ParserError:"809: unexpected token at '{\n'"> (CodeOwnership::InvalidCodeOwnershipConfigurationError)
node_modules/eslint-plugin-react/node_modules/resolve/test/resolver/malformed_package_json/package.json has invalid JSON, so code ownership cannot be determined.
Please either make the JSON in that file valid or specify `js_package_paths` in config/code_ownership.yml.
As mentioned in the docs, the default for js_package_paths is **/ which lead to node_modules being included and every installed npm package's package.json being searched. Looks like one of the packages has an intentionally malformed package.json file which caused validation to fail.
Question/Issue
I had added node_modules/**/* to unowned_globs and was confused by the fact that it did not solve the error. I did some digging into the code for this gem and found this comment where it explains that for this feature it intentionally ignores owned_globs/unowned_globs.
I ended up fixing this by explicitly setting an empty list, i.e. js_package_paths: [], to prevent the default from being used.
I was wondering what is the expected solution in this case? Is my workaround a valid way to do it or is there a better solution?
I'm also curious why unowned_globs doesn't affect js_package_paths? I found that surprising and unexpected. Perhaps the documentation could be updated to make this more explicit?
Thanks @cszatmary for the report! I am definitely on board eventually with having a way to disable this, and perhaps a better default might be to disable this unless it is explicitly enabled (although that'd be a breaking change). Since there is an easy work around, I think the easiest thing might be to just document that in the readme for now as you suggested.
As far as why unowned_globs doesn't affect that, it's mostly because that glob is to specify files that do not have/need current ownership (rather than an as an ignore list for any other arbitrary globs, such as ruby/js package paths). Eventually I wonder if we'll want something like exclude_globs that is a more general way to have code ownership ignore certain globs across the board.
What do you think of this @shageman ?
That makes sense regarding unowned_globs. I think having exclude_globs is a great idea. There are probably other situations where that would also be useful.
To me the main issue was that the behaviour wasn't obvious. Also my solution of js_package_paths: [] feels like more of a hack due to knowing details about the implementation than a proper solution. Have better documentation around this would be a great start, but I think long term doing something like exclude_globs would be a better solution.
Yeah totally agree with you @cszatmary . Are ya interested in submitting a PR to update the readme to start? Just with a note that to disable, for now, you can set js_package_paths: [] and that unowned_globs only applies to ownership of files but not ruby or js packages?
I'd be happy to do it if you don't have the time, but I always like trying to get external contributors in :)
Sorry for the delayed response, GitHub didn't notify me about your reply for some reason. I'd be happy to submit a PR to document that workaround/gotcha. Will do that shortly.
Also something else I just came across, I think another use case for an exclude_globs feature would be ignoring any autogenerated files, particularly ones that are gitignored. We have some directories we don't care about ownership for (e.g.
coverage, logs, tmp). This isn't an issue in our CI system since those files are gitignored. However, if devs want to run the codeownership validation/tooling locally we get errors about these not being owned. We'd added them to unowned_globs for now to get around this but I'd rather a proper solution were we can mark certain globs as ignored.
@cszatmary if you want to make a PR to fix this issue so that devs can run the tool locally, then we'll merge it in.