Map label to PR according to nested path #119

boaz0 · 2018-08-22T08:47:31Z

NOTE: this is an idea that I have to support this usecase. I would love to hear what you think.

Currently you can set labels according to one of the root directories (app, webpack, etc). In other words, you cannot label a PR according to a file in app/assets.

This commit adds this feature. For example in repos.yml:

theforeman/foreman:
  directory_labels:
    app/assets: ui
    app/controllers: backend
    webpack: ui

it will add ui label if there were changed files in ./app/assets/ or ./webpack/ and backend label if
a file was changed in ./app/controllers.

//cc : @ekohl @ohadlevy @tbrisker

ekohl · 2018-08-22T14:19:51Z

I'm not a fan of the nested hash. How about just a simple hash where every key is a path?

theforeman/foreman:
  directory_labels:
    app/assets: ui
    app/controllers: backend
    webpack: ui

Then you can compare it with something like:

def is_subpath_of(matcher, path)
  matcher_parts = matcher.split('/')
  path_parts = path.split('/')
  return path_parts.length >= matcher_parts.length && !matcher_parts.zip(path_parts).any? { |first, second| first != second }
end

tbrisker · 2018-08-22T14:21:52Z

Am I missing something here? why not just something like path.starts_with?(matcher)?

ekohl · 2018-08-22T14:26:22Z

Am I missing something here? why not just something like path.starts_with?(matcher)?

Is that a function of the pathing library?

[22] pry(main)> 'app/assets2'.starts_with?('app/assets')
NoMethodError: undefined method `starts_with?' for "app/assets2":String
Did you mean?  start_with?
from (pry):32:in `__pry__'

Some tests that should pass:

refute is_subpath_of('app/assets', 'app')
assert is_subpath_of('app/assets', 'app/assets')
assert is_subpath_of('app/assets', 'app/assets/bla')
refute is_subpath_of('app/assets', 'app/assets2')
refute is_subpath_of('app/assets', 'assets/app')

tbrisker · 2018-08-22T14:34:38Z

Sorry, i meant start_with? - that's a string function, https://ruby-doc.org/core-2.5.1/String.html#method-i-start_with-3F

ekohl · 2018-08-22T14:36:32Z

That would fail with app/assets2 matching on app/assets

boaz0 · 2018-08-23T05:48:44Z

We might go with @tbrisker idea if we do normalization (adding / if not giving in the end of matcher) before path.start_with?(matcher) unless I am missing an edge-case.

@ekohl thanks for your implementation, it does look way more elegant than mine. I will update the PR and add your tests.

Thanks.

tbrisker · 2018-08-23T06:47:11Z

@ekohl - Like @ripcurld suggests, that edge case could be easily solvable by adding a / at the end to make sure only the exact dir is matched. TBH, I'm not sure that edge case is one that exists in any of our code that we want these notifications for.

ekohl

This feels like a very inefficient way. A more efficient way would be to copy the mapping. If you have found a hit, you can remove any entry from the mapping where the value is that label. Then you can continue until you've either processed all files or the mapping is empty. That was also present in the old code so perhaps it's not needed to optimize it now though.

ekohl · 2018-08-29T18:09:13Z

github/pull_request.rb

@@ -179,9 +179,17 @@ def add_status(state, message, options = {})
    @client.create_status(repo.full_name, options[:sha], state, context: 'prprocessor', description: message, target_url: options[:url])
  end

+  def get_labels(filename, mapping)
+    mapping.select { |k, v| filename.start_with?(k) }.map { |k, v| v }


This should be filename == k || filename.start_with("#{k}/"). That way you can also match files. You can then remove the normalization code I think.

You can also use .values instead of the map.

Thanks @ekohl
Just one problem that I foresee - "directory_label" will be very confusing if we set there files names too. Maybe we should mention this in a document or rename it to "path_label"?

WDYT?

👍 for path_labels

ekohl · 2018-09-03T10:23:07Z

Could you rebase this on current master and also update the theforeman/foreman which now also has directory_labels?

Side note: when force pushing to a PR, GH doesn't send a notification so they can easily go unnoticed. It can be good to place a comment that you updated it.

ekohl · 2018-09-03T10:27:01Z

github/pull_request.rb

@@ -179,9 +179,17 @@ def add_status(state, message, options = {})
    @client.create_status(repo.full_name, options[:sha], state, context: 'prprocessor', description: message, target_url: options[:url])
  end

-  def set_directory_labels(mapping)
+  def get_labels(filename, mapping)
+    mapping.select { |k, v| filename == k || filename.start_with?((k+"/").squeeze) }.values


I didn't know squeeze but isn't this dangerous? Looks like it squeezes all duplicate characters but in a directory name you can have them for valid reasons (lookups would become lokups).

My bad. I thought squeeze removes characters from the end of the string.
What I wanted to do is to remove extra / in the key. For example, if someone adds "app/views/" to the map then it probably won't add "app/views/" label because the file "app/views/models/index.html" doesn't start with "app/views//".

Maybe using gsub can be replaced here?

boaz0 · 2018-09-03T11:48:03Z

@ekohl rebased and updated - can you give feedback again? thanks 😃

ekohl · 2018-09-03T11:55:05Z

config/repos.yaml

@@ -20,7 +20,7 @@ theforeman/foreman:
  redmine: foreman
  redmine_required: true
  link_to_redmine: true
-  directory_labels:
+  path_labels:
    .+-stable: Stable branch


Looks like I messed this one up. This one should have been a branch_labels. I'll submit a separate PR to fix that.

github/pull_request.rb

boaz0 · 2018-09-03T13:23:54Z

//ping @ekohl your review again 🤞

ekohl · 2018-09-03T13:53:39Z

test/config_test.rb

+      .map{ |r| r.path_labels.keys }
+      .flatten
+      .select { |file_path| file_path.end_with?('/') }
+    assert_equal [], paths, "remove backslash from the following paths: #{paths.join(', ')}"


It's a slash (/), not a backslash (\). I'd also mention it's a trailing slash. Note backslash is also in the test name.

Currently you can set labels according to one of the root directories (app, etc). In other words, you cannot label a PR according to a file in app/assets. This commit adds this feature. For example in repos.yml: theforeman/foreman: path_labels: app/assets: ui app/controllers: backend webpack: ui it will add ui label if there were changed files in ./app/assets/ or ./webpack/ and backend label if a file was changed in app/controllers NOTE: "directory_labels" options is replaced by "path_labels". Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

boaz0 · 2018-09-03T14:30:33Z

@ekohl done

boaz0 changed the title ~~[PoC] Map label to PR according to nested path~~ [WIP] Map label to PR according to nested path Aug 23, 2018

boaz0 changed the title ~~[WIP] Map label to PR according to nested path~~ Map label to PR according to nested path Aug 26, 2018

ekohl reviewed Aug 29, 2018

View reviewed changes

ekohl requested changes Sep 3, 2018

View reviewed changes

ekohl reviewed Sep 3, 2018

View reviewed changes

ekohl approved these changes Sep 3, 2018

View reviewed changes

ekohl merged commit 2eb242f into theforeman:master Sep 3, 2018

boaz0 deleted the label_matcher branch September 3, 2018 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map label to PR according to nested path #119

Map label to PR according to nested path #119

boaz0 commented Aug 22, 2018 •

edited

ekohl commented Aug 22, 2018

tbrisker commented Aug 22, 2018

ekohl commented Aug 22, 2018

tbrisker commented Aug 22, 2018

ekohl commented Aug 22, 2018

boaz0 commented Aug 23, 2018

tbrisker commented Aug 23, 2018

ekohl left a comment

ekohl Aug 29, 2018

boaz0 Aug 31, 2018

ekohl Aug 31, 2018

ekohl commented Sep 3, 2018

ekohl Sep 3, 2018

boaz0 Sep 3, 2018

boaz0 commented Sep 3, 2018

ekohl Sep 3, 2018

boaz0 commented Sep 3, 2018

ekohl Sep 3, 2018

boaz0 commented Sep 3, 2018

Map label to PR according to nested path #119

Map label to PR according to nested path #119

Conversation

boaz0 commented Aug 22, 2018 • edited

ekohl commented Aug 22, 2018

tbrisker commented Aug 22, 2018

ekohl commented Aug 22, 2018

tbrisker commented Aug 22, 2018

ekohl commented Aug 22, 2018

boaz0 commented Aug 23, 2018

tbrisker commented Aug 23, 2018

ekohl left a comment

Choose a reason for hiding this comment

ekohl Aug 29, 2018

Choose a reason for hiding this comment

boaz0 Aug 31, 2018

Choose a reason for hiding this comment

ekohl Aug 31, 2018

Choose a reason for hiding this comment

ekohl commented Sep 3, 2018

ekohl Sep 3, 2018

Choose a reason for hiding this comment

boaz0 Sep 3, 2018

Choose a reason for hiding this comment

boaz0 commented Sep 3, 2018

ekohl Sep 3, 2018

Choose a reason for hiding this comment

boaz0 commented Sep 3, 2018

ekohl Sep 3, 2018

Choose a reason for hiding this comment

boaz0 commented Sep 3, 2018

boaz0 commented Aug 22, 2018 •

edited