rubyatscale / code_ownership Goto Github PK

View Code? Open in Web Editor NEW

75.0 75.0 20.0 162 KB

A gem to help engineering teams declare ownership of code

License: MIT License

Ruby 100.00%

code_ownership's People

Contributors

Stargazers

Watchers

Forkers

kellysutton epigene taiidani cfurrow imactia jhochenbaum josecolella nate-at-gusto zaithottakath iq-scm petergoldstein wesleyw f1sherman pbomb att14 getjobber timlkelly gmacdougall

code_ownership's Issues

Running with minimal config errors

Gemfile

gem "code_ownership", "~> 1.28", require: false

config/code_ownership.yml

owned_globs:
  - app/models/**/*.rb

Running bundle exec codeownership validate produces this error:

bundler: failed to load command: codeownership (/home/ruby/apps/buyer_app/vendor/bundle/ruby/2.7.0/bin/codeownership)
Traceback (most recent call last):
	55: from /home/.asdf/installs/ruby/2.7.5/bin/bundle:23:in `<main>'
	54: from /home/.asdf/installs/ruby/2.7.5/bin/bundle:23:in `load'
	53: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/exe/bundle:36:in `<top (required)>'
	52: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
	51: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/exe/bundle:48:in `block in <top (required)>'
	50: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/cli.rb:25:in `start'
	49: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
	48: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/cli.rb:31:in `dispatch'
	47: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
	46: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	45: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
	44: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/cli.rb:484:in `exec'
	43: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:23:in `run'
	42: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:58:in `kernel_load'
	41: from /home/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/bundler-2.3.4/lib/bundler/cli/exec.rb:58:in `load'
	40: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/bin/codeownership:23:in `<top (required)>'
	39: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/bin/codeownership:23:in `load'
	38: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/bin/codeownership:5:in `<top (required)>'
	37: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/cli.rb:11:in `run!'
	36: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/cli.rb:63:in `validate!'
	35: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/_methods.rb:270:in `block in _on_method_added'
	34: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `validate_call'
	33: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `bind_call'
	32: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership.rb:59:in `validate!'
	31: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/_methods.rb:270:in `block in _on_method_added'
	30: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `validate_call'
	29: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `bind_call'
	28: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:44:in `validate!'
	27: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:44:in `flat_map'
	26: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:44:in `each'
	25: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:45:in `block in validate!'
	24: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/_methods.rb:270:in `block in _on_method_added'
	23: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `validate_call'
	22: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `bind_call'
	21: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private/validations/files_have_owners.rb:14:in `validation_errors'
	20: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/_methods.rb:270:in `block in _on_method_added'
	19: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `validate_call'
	18: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `bind_call'
	17: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:111:in `files_by_mapper'
	16: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:111:in `each'
	15: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private.rb:112:in `block in files_by_mapper'
	14: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/_methods.rb:270:in `block in _on_method_added'
	13: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `validate_call'
	12: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `bind_call'
	11: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private/ownership_mappers/js_package_ownership.rb:32:in `map_files_to_owners'
	10: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/_methods.rb:270:in `block in _on_method_added'
	 9: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `validate_call'
	 8: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation.rb:161:in `bind_call'
	 7: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private/parse_js_packages.rb:53:in `all'
	 6: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private/parse_js_packages.rb:53:in `map'
	 5: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private/parse_js_packages.rb:54:in `block in all'
	 4: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation_2_7.rb:106:in `block in create_validator_method_fast1'
	 3: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/sorbet-runtime-0.5.10172/lib/types/private/methods/call_validation_2_7.rb:106:in `bind_call'
	 2: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/code_ownership-1.28.0/lib/code_ownership/private/parse_js_packages.rb:23:in `from'
	 1: from /home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/json-2.3.1/lib/json/common.rb:263:in `parse'
/home/ruby/apps/my_app/vendor/bundle/ruby/2.7.0/gems/json-2.3.1/lib/json/common.rb:263:in `parse': 784: unexpected token at '{ (JSON::ParserError)
'

Have way to disable Javascript Package Ownership

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?

Unclear what configurations are required

Hi, I'm having some trouble parsing from the README what the required configuration files are and how they should be structured.

It looks like we require a
config/code_ownership.yml
config/teams/my_team.yml

I'm not entirely sure how to structure those files.

Performance Improvement Ideas

It's important for code_ownership to be fast in various contexts. Here are different situations we want to be fast:

When getting code ownership for a single class/file in production. This is important at Gusto since we tag requests and async jobs with code ownership, so we don't want to add a lot of overhead here.
When running code ownership validations locally. We want this to be fast when run on all files OR on a subset of files in the commit hook

On very large codebases, the slowest part of this is file annotations – reading all files and checking for annotations can be slow. I wanted to share a laundry list of some ideas for improvement here:

Switch to team-based annotations for these files and give the option to turn off file annotations.

Pro: This would be much faster
Con: A possibly worse UX for annotating file ownership

Switch file annotations to run after checking for ownership via packages and team globs

Pros: This will often short circuit the more files that are owned in other ways
Cons: This changes public API (we can possibly defer this by allowing code_ownership.yml to specify an order that mappers run in. This wouldn't improve the speed of generating code owners since we ask all mappers to show ownership for all files.

Have mappers "water fall down" the files they have already mapped. This would allow file annotations to not need to look at files we already know we have owners for.

Pros: This could speed a lot of different things up.
Cons: Increases complexity

Have file annotations open and read files in parallel

Pros: File IO is a great parallelizable task, so it might be well suited
Cons: Adds a potentially complex dependency. All of codenwership is also already often run in parallel in the context of commit hooks.

Separately, we should consider a test/build step which measures code ownership speed on a large system. At Gusto we might include a post-build step which sends a metric to datadog about how long different commands take to run so we can track and observe changes over time.

Broken link on blog article

Hello! This isn't about the code_ownership gem, but I couldn't find another way to report this. On https://engineering.gusto.com/laying-the-cultural-and-technical-foundation-for-big-rails/ the GitHub links point to the GitHub organization "bigrails" which no longer exists. If you navigate to a specific repository you'll get redirected to the new location under "rubyatscale" but if you follow the link to the org at the bottom of the post you'll get a 404. Also, near the bottom of the article it says "Leave a comment on this blog post" but I'm not seeing any way to leave a comment. There is no contact information for the author of the blog post so I wasn't sure how else to get in touch. Thanks for the article and these gems—they will be useful to my team!

Refactor out a "glob based ownership" mapper helper

Packwerk package based, JS package based, and team globs all use a very common set of patterns for specifying ownership.

We might consider extracting out a common base for glob-based ownership mapper strategies. This could then be used by future mappers, such as one to specify ownership for gems the application uses.

Missing install documentation for `bin/codeownership`

Hello!

I am trying to follow the instructions for setting up this gem. After adding the gem to my Gemfile, creating a teams folder, and a config/code_ownership.yml, I can run CodeOwnership.validate! in the Rails console. However, bin/codeownership validate results in no such file or directory: bin/codeownership.

I'm wondering if there is a missing step in the instructions or if I have misunderstood the instructions. Thanks!

Thread safety

Class instance variables should use Concurrent::Map or similar to prevent corruption

File annotation and Ruby "magic" comments

It seems that code_ownership requires that the file annotation be a comment on the very first line of a Ruby file, but this conflicts with Ruby "magic" comments, like # frozen_string_literal: true, which is also required to be on the first line of a file.

When digging into the code of code_ownership, I see a comment that alluded to an idea that it used to (?) check the first two lines of a Ruby file for the annotation, but the code does not align with that comment anymore.

... if the annotation isn't in the first two lines ...

Code lines: https://github.com/rubyatscale/code_ownership/blob/main/lib/code_ownership/private/ownership_mappers/file_annotations.rb#L57-L61

sig { params(filename: String).returns(T.nilable(CodeTeams::Team)) }
def file_annotation_based_owner(filename)
  # If for a directory is named with an ownable extension, we need to skip
  # so File.foreach doesn't blow up below. This was needed because Cypress
  # screenshots are saved to a folder with the test suite filename.
  return if File.directory?(filename)
  return unless File.file?(filename)

  # The annotation should be on line 1 but as of this comment
  # there's no linter installed to enforce that. We therefore check the
  # first line (the Ruby VM makes a single `read(1)` call for 8KB),
  # and if the annotation isn't in the first two lines we assume it
  # doesn't exist.

  line_1 = File.foreach(filename).first

  return if !line_1

  begin
    team = line_1[TEAM_PATTERN, :team]
  rescue ArgumentError => ex
    if ex.message.include?('invalid byte sequence')
      team = nil
    else
      raise
    end
  end

  return unless team 
  #...snip
end

Migrate `code_ownership` to use `packs` standard

Add method to return ownership adoption (percentage of files with ownership)

One key metric to track with any project is how the percentage of files with ownership is changing over time. This can be accomplished now using the interfaces under Private. Do you think it would be reasonable to implement something like this?

def percentage_of_files_with_ownership(files: Private.tracked_files)
  files = (Private.tracked_files & files)
  allow_list = Dir.glob(Private.configuration.unowned_globs)

  files_by_mapper = Private.files_by_mapper(files)
  files_not_mapped_at_all = files_by_mapper.select { |_file, mapper_descriptions| mapper_descriptions.count == 0 }.keys

  files_without_owners = (files_not_mapped_at_all - allow_list).count
  files_tracked_count = (files_by_mapper.keys - allow_list).count

  with_owners_count = files_tracked_count - files_without_owners

  (with_owners_count / files_tracked_count.to_f) * 100
end

(Using code from 1.32.2 for the sake of demonstration)

cc @alexevanczuk
Let me know what you think and I will get a PR ready.