-
Notifications
You must be signed in to change notification settings - Fork 74
Remove prepend_namespace #551
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
Conversation
We do not use prepend namespace The feature is causing us to double the number of queries when looking up objects
Checked commit kbrock@0885f24 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
Oh interesting. So the vendor part of this was part of an idea to create namespaces for plugins. Plugins could then own their own namespace or even domain, and then provide concrete implementations of things like provisioning and event handling, where the core automation engine would have generic stuff and call into plug-in specific stuff. I don't believe this was ever completed, but do you see any vendor specific namespaces in manageiq-content? cc @agrare |
I don't recognize any of this but I like the idea of moving provider specific automate bits into their respective plugins |
@Fryguy I didn't look. I remember you saying that Tina introduced this but we never ended up using it (6 months ago when I was tracing automate), and I saw a branch yesterday and finally pushed it forward. When tracing the queries and walking through the code, it is a distraction to me, especially knowing they are useless queries. |
Yeah my only concern is that there is already some vendor namespaces built, that this would effectively break. If not, then I think we can merge. |
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
2 similar comments
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
@agrare Do you know if we've been using vendor specific namespaces in automate? I think we may for vmware, but was unsure how to even determine this. |
@kbrock I don't know what this is or if it is used unfortunately :/ |
Oh it's from an old feature idea to make automate pluggable by provider...I can't remember if anything ever got implemented, but I'm pretty sure it didn't |
@Fryguy agreed. You had told me you had suggested the feature but it wasn't implemented and could probably tear it out. So I added this PR to remove all these extra queries. Lookups end up taking 3 queries instead of 1 (for the non-missing case) |
I'm guessing we wanted to put this into Didn't check if our rake tasks checks all plugins for
|
It definitely wasn't implemented at the plugin level. I thought Tina might have put some in manageiq-content, but I'm not sure. This is the list of provider specific stuff I could find, but I don't think any of it is designed around the prepend_namespace idea, because I think the provider name would have to be a top-level namespace right under the domain. @agrare Can you verify the purpose of these?
|
Yeah, none of these are at the toplevel - and the code prepends the vendor name as the namespace, so none of these fall in that category. Merging. |
Well |
We do not use
prepend_namespace
The feature is causing us to double the number of queries when looking up objects
@Fryguy you mentioned that we are not using this.
It was an extension mechanism added by tina that seems to not be leveraged.
I just put this into a PR so I didn't have to have a local branch and have to remember where I saw this issue.