[WIP] extend relationships to define the appropriate class names #654
Conversation
kbrock
requested review from
agrare,
andyvesel,
aufi,
Fryguy and
mansam
as code owners
kbrock
force-pushed
the
local_child_managers
branch
2 times, most recently
from
7e3576e
to
18aa014
Compare
| before_create :ensure_managers | ||
|
|
||
| # TODO: move to before_validation | ||
| before_update :ensure_managers_zone_and_provider_region |
There was a problem hiding this comment.
One thing to keep in mind, the tenant_id is set also in a before_validation callback so we need to make sure those play nicely together. Don't want to be setting the child.tenant_id = tenant_id before tenant_id has been set
https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/tenancy_mixin.rb#L5
| @@ -419,52 +427,19 @@ def ensure_managers | |||
| ensure_network_manager | |||
| ensure_cinder_manager | |||
| ensure_swift_manager | |||
| # TODO: remove when this moves to before_initialization | |||
There was a problem hiding this comment.
I think we should create the objects at initialization time - empty vessels
then in before validation, make sure they have the correct values
I seem to remember that rails could auto build the has_one for us but can't find the references anymore.
But maybe before validation is where we will create these objects too
| network_manager.tenant_mapping_enabled = tenant_mapping_enabled | ||
| network_manager.save! | ||
| def ensure_managers_zone_and_provider_region | ||
| child_manager_references.each do |child_manager| |
There was a problem hiding this comment.
If we could work out the list of child managers generically we could do this in core right?
There was a problem hiding this comment.
I had child_managers but that messed with an already defined child_managers relation. I'm guessing that is what you are referring.
Were, we want to access the actual child managers that we are using.
If we want to stick with the child_managers association, then we could define swift_manager to be a select into child_managers or something like that?
just having a child managers relation and a different association for swift_manager messes things up.
|
Hey @kbrock now that ManageIQ/manageiq#20739 is merged can we get this in? |
|
#673 was merged moving the SwiftManager from core to this plugin, which also made the class names more consistent. |
|
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
|
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 |
kbrock
force-pushed
the
local_child_managers
branch
2 times, most recently
from
cdf8136
to
0311d4d
Compare
WIP: quite a bit of stuff has happened since then. there is a HasNetworkManagerMixin that may meet many of the needs WIP: do a diff with origin to make sure the rebase didn't change too much seems quite a few concepts have been changed in master so definitely need to re-evaluate this use parent definition of ensure since they are now pretty generic. They may be no longer necessary and a build_ may suffice. Use parent definition of propagation so all attributes go across Extend parent managers to group the functionality together WIP: quite a bit of stuff has happened since then. there is a HasNetworkManagerMixin that may meet many of the needs
kbrock
force-pushed
the
local_child_managers
branch
from
0311d4d
to
4b61465
Compare
|
Checked commit kbrock@4b61465 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint app/models/manageiq/providers/openstack/infra_manager.rb
|
|
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 |
This depends upon:
putting in this PR to show where I would like to go and to run cross repo tests
use parent definition of ensure since they are now pretty generic.
They may be no longer necessary and a build_ may suffice.
Use parent definition of propagation so all attributes go across
Extend parent managers to group the functionality together