-
Notifications
You must be signed in to change notification settings - Fork 991
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.
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
Fixes #29450 - Use sp-references in used taxonomy lookup #9976
Fixes #29450 - Use sp-references in used taxonomy lookup #9976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read correctly, this PR will hide proxies that are not associated with at least one host in the current org/location.
I think it will make non-associated proxies to entirely disappear.
@adamruzicka Is this your original intention?
@@ -7,8 +7,12 @@ def smart_proxy_reference(hash) | |||
ProxyReferenceRegistry.add_smart_proxy_reference hash | |||
end | |||
|
|||
def smart_proxy_scope(hosts_scope = Host::Managed.all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: we can make it a scope:
def smart_proxy_scope(hosts_scope = Host::Managed.all) | |
scope :with_smart_proxies, -> { eager_load(ProxyReferenceRegistry.smart_proxy_references.select(&:join?).map(&:join_relation) } |
I have dereferenced the proxy_join_tables
method. I suppose we can reuse it, if the method will be defined as static.
then later we can use it directly:
hosts_scope.with_smart_proxies.pluck(proxy_column_list).flatten.uniq.compact
6a51a7d
to
a021926
Compare
That sounds about right |
What exact roles or permissions did you give to that user? If I give the user the org admin role then I can reproduce both with and without this patch. Apparently as long as the user has a role grants them the |
I have tried it with org_admin role indeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ares This one looks good to me. Any additional concerns before I merge it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @adamruzicka and @ShimShtein !
@adamruzicka, could you please squash the commits? |
Before we relied only on Puppet and Puppet CA proxy features and searched for hosts which were linked against the proxy in question using host.puppet_proxy_id and host.puppet_ca_proxy_id fields. When the proxy didn't have any of these features, the search was unbounded. This meant we took all the hosts and taxonomy ids from them. This lead to proxy seemingly being assigned to taxonomies when it really wasn't and not being able to remove the proxy from such taxonomies. After this change, we leverage the smart proxy references framework to really look for hosts reference a given proxy in any way.
a2ce493
to
0577346
Compare
TODOs:
SmartProxy#taxonomy_foreign_conditions
which is wrong anyway?Fixes #37028 - Set up sp-reference for content source Katello/katello#10833
Fixes #37027 - Set up sp-reference to scap proxy foreman_openscap#555