-
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
Support container/infra/cloud provider policies in the UI #11002
Support container/infra/cloud provider policies in the UI #11002
Conversation
63f007f
to
162b83e
Compare
023cb47
to
d503ffe
Compare
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
baa9333
to
f999f1a
Compare
@@ -27,6 +27,9 @@ def self.supported_types_and_descriptions_hash | |||
|
|||
belongs_to :provider | |||
belongs_to :tenant | |||
include ComplianceMixin | |||
include CustomAttributeMixin | |||
after_initialize :load_custom_attributes |
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.
This scares me. after_initialize is a harsh penalty. Also, this will be called for each individual EMS even if you don't need 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.
@alongoldboim you probably want to move this functionality down to the container manager. If we need to expose this type of relationship for all managers, we can look at that later.
Also, miq_report
, another class that uses load_custom_attributes_for
via the CustomAttributeMixin
, uses lazy loading to pull in the custom attributes only when they're accessed.
If there's definitely not a way to do this lazily, then at least having it isolated in the container manager would be better than exposing in ext_management_system
.
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.
@alongoldboim you probably want to move this functionality down to the container manager. If we need to expose this type of relationship for all managers, we can look at that later.
@alongoldboim @blomquisg idea was to bring this functionality to all type of providers. If this changed then it needs to be properly advertised to everyone (it's very important that it is going to be clear).
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.
in this PR(#11369 ) is loading of virtual custom attributes moved to constructor MiqExpression, @alongoldboim is it enough for your case ?
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.
@Fryguy the idea is to use lazy loading as @lpichler suggested, just need to make sure it works for me in all the cases.
@blomquisg I think we should enable policies for all kinds of providers, do you see a reason why not to do it?
@lpichler going over that patch, will try it later on today, thanks for the help.
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.
@lpichler seems to work well 👍
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.
@alongoldboim so this #11369 needs to be merged first ?
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.
Yep, added a depends on comment.
8f4240a
to
fe03957
Compare
@miq-bot remove_label wip |
depends on: #11369 |
@Fryguy Looks OK now? |
b4016c5
to
149e304
Compare
@h-kataria any idea why the tests are failing? |
149e304
to
76154da
Compare
@h-kataria tests are passing, anything else is missing? :) |
end | ||
process_emss(emss, "check_compliance") | ||
showlist ? show_list : show | ||
emss.count |
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.
@alongoldboim why do we need emss.count here, i don't see that being used.
:task => "Compliance Check"}, :error) | ||
end | ||
process_emss(emss, "check_compliance") | ||
showlist ? show_list : show |
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.
@alongoldboim dont need to set showlist variable as it is not being used anywhere else, you could just do
@lastaction == "show_list" ? show_list : show
76154da
to
a40ee2b
Compare
@h-kataria fixed. |
Checked commits alongoldboim/manageiq@d25120a~...a40ee2b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/controllers/application_controller/tree_support.rb
app/controllers/ems_common.rb
app/controllers/miq_policy_controller.rb
|
looks good now. |
@alongoldboim compliance events need icons: |
@alongoldboim is there a BZ for this? |
Alon left the team. The feature was motivated by "CM-Ops" effort, probably doesn't have a BZ. @simon3z? The missing icons I mention above also don't have BZ. And while I look at it, shouldn't say "ExtManagementSystem". => ManageIQ/manageiq-ui-classic#94, #13388. |
Support container/infra/cloud provider policies in the UI (cherry picked from commit 6434490) https://bugzilla.redhat.com/show_bug.cgi?id=1411364
Euwe backport details:
|
Enable provider policies definitions.
Control panel ( added option for providers policies )
Control panel Add Policy
Providers panel ( added policies actions )