Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move unassignable and unremovable labels from constants to settings #485

Merged
merged 1 commit into from
Mar 13, 2020
+14 −7
Merged

Move unassignable and unremovable labels from constants to settings #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/settings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ bugzilla:
product:
grafana:
url:
labels:
unassignable: {}
unremovable: []

# Worker settings
diff_content_checker:
Expand Down
6 changes: 6 additions & 0 deletions config/settings/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
:labels:
:unassignable:
jansa/yes: jansa/yes?
:unremovable:
- jansa/no
- jansa/yes
# In test, turn everything on by default
# NOTE: Using empty string is a HACK until we can get
# https://github.com/danielsdeleo/deep_merge/pull/33 released via the
Expand Down
8 changes: 4 additions & 4 deletions lib/github_service/commands/add_label.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ module Commands
class AddLabel < Base
include IsTeamMember

UNASSIGNABLE = {
"jansa/yes" => "jansa/yes?"
}.freeze
def unassignable_labels
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already cached in Settings and that should get reloaded any time the settings are changed. Are you concerned about the call to stringify_keys?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By that logic, then we just shouldn't be caching it at all, and makes me wonder why you have a @ var there in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the cache here so that we don't have to call stringify_keys for every label in the command.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other cache was not necessary, so I removed it.

Copy link
Member

@NickLaMuro NickLaMuro Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, to be clear:

I don't really see this as a big performance hit, since we would be caching it for it's once use per instance of this (it seems), so I am more commenting on the fact that we are caching this on the instance, but it's cache should never be made use of since this is only ever called once per instance.

If we still want the cache, then probably cache_with_timeout would make sense based on the use case that you described: changing the files and having it auto reload without a server restart. That said, I have found that the listen gem has caused my CPU to spike to 100% on my current machine, so I tend to avoid trying to relying on that gem/functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the cache here so that we don't have to call stringify_keys for every label in the command.

Correction: You are right with this since it is being called with in a loop. I would argue that it is negligible with the number of labels we currently have in this list and the number of labels that are passed in for add_label in general (one or two), and there is more savings to be had by caching this on a class level is that is your true intent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I have found that the listen gem has caused my CPU to spike to 100% on my current machine, so I tend to avoid trying to relying on that gem/functionality.

Can you open an issue for this...I'd not heard this before and I don't have this issue nor do we see it in prod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the memoization is fine since this is at an instance level. The next instance of AddLabel will query the Settings again.

@unassignable_labels ||= Settings.labels.unassignable.to_h.stringify_keys
end

private

Expand Down Expand Up @@ -57,7 +57,7 @@ def correct_invalid_labels(valid_labels, invalid_labels)

def handle_unassignable_labels(valid_labels)
valid_labels.map! do |label|
UNASSIGNABLE.key?(label) ? UNASSIGNABLE[label] : label
unassignable_labels.key?(label) ? unassignable_labels[label] : label
end
end

Expand Down
4 changes: 1 addition & 3 deletions lib/github_service/commands/remove_label.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ module Commands
class RemoveLabel < Base
include IsTeamMember

UNREMOVABLE = %w[jansa/yes jansa/no].freeze

alias_as 'rm_label'

private
Expand Down Expand Up @@ -40,7 +38,7 @@ def extract_label_names(value)

def process_extracted_labels(issuer, valid_labels, _invalid_labels, unremovable)
unless triage_member?(issuer)
valid_labels.each { |label| unremovable << label if UNREMOVABLE.include?(label) }
valid_labels.each { |label| unremovable << label if Settings.labels.unremovable.include?(label) }
unremovable.each { |label| valid_labels.delete(label) }
end
end
Expand Down