Move unassignable and unremovable labels from constants to settings #485
Conversation
| @@ -3,7 +3,9 @@ module Commands | |||
| class RemoveLabel < Base | |||
| include IsTeamMember | |||
|
|
|||
| UNREMOVABLE = %w[jansa/yes jansa/no].freeze | |||
| def unremovable_labels | |||
There was a problem hiding this comment.
Maybe make this a class variable so we aren't loading this on every instance?
There was a problem hiding this comment.
Of course, if needed, you can alias it as an instance method, but I think that it should still be cached on the class level.
| UNASSIGNABLE = { | ||
| "jansa/yes" => "jansa/yes?" | ||
| }.freeze | ||
| def unassignable_labels |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I added the cache here so that we don't have to call stringify_keys for every label in the command.
There was a problem hiding this comment.
The other cache was not necessary, so I removed it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think the memoization is fine since this is at an instance level. The next instance of AddLabel will query the Settings again.
|
@bdunne I have a larger concern in general about over using Should I open an issue about this to get a discussion going? |
Might be a good idea. Currently the settings override on the box contains a mix of secrets and org-specific configurations. I would be open to splitting the current config file into two files (one for secrets, one for everything else). I'm not sure where we would put the new "everything else" file because I don't think that belongs in this repo. @Fryguy Any thoughts? |
These values really belong to the manageiq organization and the bot is intended to be reusable by people in other organizations. Prefer settings for these types of values rather than hard-coded constants.
bdunne
force-pushed
the
move_static_labels_to_setings
branch
from
c071dd0 to
0444c41
Compare
|
Checked commit bdunne@0444c41 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint **
|
That sounds good to me. The general idea is that while this is called the miq_bot, it's designed more generally than that (or at least should be). I've had aspirations to move this code to a more generic bot system that others could use. |
|
Opened said issue here: Though, doesn't quite address the thoughts expressed here:
I don't mind if the conversation there expands to design ideas on how we would approach something like that. |
These values really belong to the ManageIQ organization and the bot is intended to be reusable by people in other organizations. Prefer settings for these types of values rather than hard-coded constants.
cc @NickLaMuro