Modularize MiqServer #15014
Modularize MiqServer #15014
Conversation
NickLaMuro
force-pushed
the
miq_server_module_refactor
branch
from
5e58c57
to
e84dcc5
Compare
Moved the methods into app/models/miq_server.rb and app/models/miq_server/worker_management/quiesce.rb since each of those files are basically the ones that use them. `kill_all_workers` will actually be moved in a later commit to modularize the core methods of MiqServer.
Moves them into a module for reusablity. Freezes ones that haven't been frozen already.
NickLaMuro
force-pushed
the
miq_server_module_refactor
branch
from
e84dcc5
to
65e46fa
Compare
Moves common methods out of MiqServer to be reused in a later commit.
Moves "srsly everything" in app/models/miq_server/worker_management.rb into it's own module for reuse. Used a concern here since it seems to handle things better, but can't use `include_concern` because that doesn't support non-standard paths (yet...)
Allows MiqServerQueueManagement to be reused without loading MiqServer
NickLaMuro
force-pushed
the
miq_server_module_refactor
branch
from
65e46fa
to
41d2f08
Compare
|
Checked commits NickLaMuro/manageiq@7214dac~...41d2f08 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/miq_server/base_methods.rb
|
| RESTART_EXIT_STATUS = 123 | ||
| include MiqServerBaseConstants | ||
| include MiqServerBaseMethods | ||
| include MiqServerQueueManagement |
There was a problem hiding this comment.
These should be MiqServer::BaseConstants, etc. based on your file structure and that there are other modules already using that pattern. Also, the requires at the top probably won't be needed then as autoload should pick them up at the correct location.
There was a problem hiding this comment.
This is not possible, but I guess I didn't explain (thought I did somewhere, but I will make sure I add to the description at some point).
Basically, it boils down to this: MiqServer is a class that inherits from ApplicationRecord, not a module, and because of load ordering, that always gets loaded first before any of the sub modules in the app/models/miq_server folder. For what I am doing with #14916, I don't want to have portions of this require ApplicationRecord to be loaded, so this needs to be it's own module, but because of that, it can't live within the same namespace. Here is why:
module MiqServer
module BaseConstants
# code here
end
end
class MiqServer < ApplicationRecord
end
#=> Error: MiqServer is already defined as a moduleIf we were to do it in the inverse, then we would have to define MiqServer as a class inheriting from ApplicationRecord, which doesn't work with #14916 where I am trying to define it as vanilla active record.
I will push what I have to #14916 where I implement this and the other relevant PRs so you can see how I plan to use this. That commit (and the rest of the PR really...) is very POC and temporary at the moment, but it should provide an idea for why I did it this way.
My opinion is just namespace as I mentioned and do |
This is a commit to help implement the features implemented in the
following PRs:
ManageIQ#14953
ManageIQ#14967
ManageIQ#15014
ManageIQ#15018
ManageIQ#15019
ManageIQ#15020
Meant to illustrate how they will eventually be used in this PR, and
show how they will be implemented when they are merged into master.
The prior commits to this one are left in place to show before and
after, but this and those will be removed in the final iteration of this
pull request, and only the necessary changes needed to implement this in
the Rake tasks will be part of this PR.
Applying all of the changes for this PR was done as follows:
$ git apply <(curl -L ManageIQ#14953)
$ git apply <(curl -L ManageIQ#14967)
$ git apply <(curl -L ManageIQ#15014)
$ git apply <(curl -L ManageIQ#15018)
$ git apply <(curl -L ManageIQ#15019)
$ git apply <(curl -L ManageIQ#15020)
And then with some extra changes on top of that:
* Making use of `Miq.env` and `Miq.root` where relevant
* Implementing the modulularized PRs in the `tools/utils/mini_*.rb`
files where relavant
* Various bug fixes (though some seem like they shouldn't be there...)
Again, this is meant as a demonstration commit to show the before and
after while this is still a POC. Once the necessary pieces are in place
on master to make this commit irrelevant, it will be rebased out. It
may also change over time as this PR is worked on further.
Similar to my other answer, I would then namespace everything under See NickLaMuro@93e2dd4 from #14916 as an example of how I plan to implement this and the other related PRs. |
|
I see. Sounds solid to me then, we do need to stop loading the world (and eventually, IMO, Rails) to do anything. Given your explanation, I like the |
|
@Fryguy bump |
1 similar comment
|
@Fryguy bump |
|
This pull request is not mergeable. Please rebase and repush. |
|
This pull request has been automatically closed because it has not been updated for at least 6 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! |
Moves portions of MiqServer code into modules that can be consumed without requiring
app/models/miq_server.rbto be loaded. See #14916 for details on it's usage.Alternative Consideration
Since we already have a
app/models/mixinsdirectory, perhaps it makes sense for this code to live there? Effectively what we are doing here is what we are making use of there, but haven't done so in a large scale like this.I was considering moving the entire
app/models/miq_server/directory intoapp/models/mixin/miq_server/directory, and namespacing everything undermodule MiqServerMixin, but didn't do to stay similar to other PRs in this vain. That said, this might be a better route to consider.Links