-
Notifications
You must be signed in to change notification settings - Fork 898
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
Tenant changes changes miq product features on all servers #20772
Conversation
@@ -317,6 +318,10 @@ def create_miq_product_features_for_tenant_nodes | |||
MiqProductFeature.seed_single_tenant_miq_product_features(self) | |||
end | |||
|
|||
def update_miq_product_features_for_tenant_nodes | |||
MiqProductFeature.invalidate_caches_queue |
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.
Do we want to be more selective about which changes need to update the product features?
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.
I had asked Libor the same question.
It would mean that we know in this class exactly what the miq product feature cache is holding. and if the business logic or display logic over there changes.
I deemed that we don't make many tenant changes other than name adds and removes, so the blanket change was the safest bet and not overly cache invalidation happy.
If you have another interpretation, please share
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.
I'll put in after_create per your other request
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.
So my argument over there was that the logic doesn't belong in the UI but did on the model, so at least all of the "information" would be self contained... But I agree that you'd have to have model logic to know this relationship anyway and then remember to update both sides, and that's not ideal.
Overall LGTM...can you also add a "Closes ManageIQ/manageiq-api#928" somewhere so we know this is linked to that and will auto-close it? |
According to the issue described in ManageIQ/manageiq-api#928, perhaps this should only be a part of |
ab4bf20
to
817e4a7
Compare
@Fryguy made that change - let me know if you have other ideas |
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.
LGTM 👍
@kbrock Can you rebase, and then I can merge? (Didn't realize I hadn't merged this yet :( ) |
miq product features contain tenant data. So we want to update the product features data on all servers when a tenant changes This allows us to remove special logic from the front end. ManageIQ/manageiq-ui-classic#7430
817e4a7
to
c7aa203
Compare
Checked commits kbrock/manageiq@92cb362~...c7aa203 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint |
@Fryguy thanks for the ping. I hadn't realized this had a conflict |
Backported to ivanchuk via #20886 |
Tenant changes changes miq product features on all servers (cherry picked from commit c7836b2)
Jansa backport details:
|
Tenant changes changes miq product features on all servers (cherry picked from commit c7836b2)
Kasparov backport details:
|
alternative for: #20708 (first commit is from that PR)
miq product features contain tenant data. So we want to update the product features data
on all servers when a tenant is added or changes.
This allows us to remove special logic from the front end: ManageIQ/manageiq-ui-classic#7430
Closes ManageIQ/manageiq-api#928
Closes #20708