On 06/08/2017 02:33 AM, Josef Reidinger wrote: > On Wed, 7 Jun 2017 15:38:45 -0500 > Goldwyn Rodrigues <[email protected]> wrote: > >> From: Goldwyn Rodrigues <[email protected]> >> >> This is a precursor to use aa-status json profile view. >> >> Please note, "Configure" option is left hanging which will be filled >> in the next patch. > > Well, to be honest I found discussion in pull request easier to orientate and > also for discussion, but lets comment it here.
Sorry, coming from the kernel development community, I assumed a bigger subscription here and hence more eyeballs. I would perform a pull request in the next review cycle. > >> >> Signed-off-by: Goldwyn Rodrigues <[email protected]> >> --- >> src/clients/apparmor-settings.rb | 139 >> ++++++++++++++++++++++++--------------- >> 1 file changed, 85 insertions(+), 54 deletions(-) >> >> diff --git a/src/clients/apparmor-settings.rb >> b/src/clients/apparmor-settings.rb >> index ecea5bc..2b2d3f2 100644 >> --- a/src/clients/apparmor-settings.rb >> +++ b/src/clients/apparmor-settings.rb >> @@ -1,8 +1,6 @@ >> -# encoding: utf-8 >> - >> # >> *************************************************************************** >> # >> -# Copyright (c) 2002 - 2012 Novell, Inc. >> +# Copyright (c) 2017 SUSE Linux >> # All Rights Reserved. >> # >> # This program is free software; you can redistribute it and/or >> @@ -17,71 +15,104 @@ >> # You should have received a copy of the GNU General Public License >> # along with this program; if not, contact Novell, Inc. >> # >> -# To contact Novell about this file by physical or electronic mail, >> -# you may find current contact information at www.novell.com >> +# To contact SuSE about this file by physical or electronic mail, >> +# you may find current contact information at www.suse.com >> # >> # >> *************************************************************************** >> -module Yast >> - class ApparmorSettingsClient < Client >> - def main >> - Yast.import "UI" >> - >> - textdomain "yast2-apparmor" >> >> - # The main () >> - Builtins.y2milestone("----------------------------------------") >> - Builtins.y2milestone("AppArmor module started") >> +require "yast" >> >> - Yast.import "Label" >> - Yast.import "Popup" >> - Yast.import "Wizard" >> +Yast.import "UI" >> +Yast.import "Label" >> +Yast.import "Popup" >> +Yast.import "Service" >> >> - Yast.include self, "apparmor/apparmor_packages.rb" >> - Yast.include self, "apparmor/aa-config.rb" >> +module AppArmor >> + class ApparmorSettings >> + include Yast::I18n >> + include Yast::UIShortcuts >> >> - # no command line support #269891 >> - if Ops.greater_than(Builtins.size(WFM.Args), 0) >> - Yast.import "CommandLine" >> - CommandLine.Init({}, WFM.Args) >> - return >> + def initialize >> + @service_enabled = Yast::Service.Enabled("apparmor") >> + end >> + def run >> + return unless create_dialog >> + begin >> + return event_loop >> + ensure >> + Yast::UI.CloseDialog >> end > > I really suggest to use Dialog class as ancestor, it will make your life > easier: > http://www.rubydoc.info/github/yast/yast-yast2/UI/Dialog > > it already have similar code implemented. > >> - >> - return if !installAppArmorPackages >> - >> - @config_steps = [ >> - { "id" => "apparmor", "label" => _("Enable AppArmor Functions") } >> - ] >> - >> - @steps = Builtins.flatten([@config_steps]) >> - >> - @current_step = 0 >> - @button = displayPage(@current_step) >> - >> - # Finish >> - Builtins.y2milestone("AppArmor module finished") >> - Builtins.y2milestone("----------------------------------------") >> - >> - # EOF >> - >> - nil >> end >> >> - def displayPage(no) >> - current_id = Ops.get_string(Ops.get(@steps, no), "id", "") >> - button = nil >> - >> - UI.WizardCommand(term(:SetCurrentStep, current_id)) >> - >> - if current_id == "apparmor" >> - #button = displayAppArmorConfig(); >> - button = displayAppArmorConfig >> + private >> + def create_dialog >> + Yast::UI.OpenDialog( >> + Opt(:decorated, :defaultsize), >> + VBox( >> + Heading(_("Apparmor Settings")), >> + VSpacing(1), >> + VBox( >> + CheckBox(Id(:aaState), Opt(:notify), _("&Enable Apparmor"), >> @service_enabled) >> + ), >> + VSpacing(1), >> + Frame( >> + Id(:aaEnableFrame), >> + _("Configure Profiles"), >> + HBox( >> + Label(_("Configure Profile modes")), >> + PushButton(Id(:modeconf), _("Configure")), >> + ) >> + ), >> + VSpacing(1), >> + HBox( >> + Right( >> + PushButton(Id(:quit), Yast::Label.QuitButton) >> + ) >> + ) >> + ) >> + ) >> + Yast::UI.ChangeWidget(Id(:aaEnableFrame), :Enabled, @service_enabled) >> + end > > I suggest to use rubocop for checking coding style style. Here for example > missing empty line, which is > quite hard to read. For example rubocop config please see: > > https://github.com/yast/yast-bootloader/blob/master/.rubocop.yml > > for usage just run rubocop command. Hint: `rubocop --auto-correct` will fix > coding style where it is safe. > >> + def event_loop >> + loop do >> + case Yast::UI.UserInput >> + when :modeconf >> + break >> + when :quit >> + break > > you can write it as `break if [:modeconf, > :quit].include?(Yast::UI.UserInput)` > but I suggest to store UserInput outside of it as it have side-efect, > so it should be more visible. > >> + end >> + @service_enabled = Yast::UI.QueryWidget(:aaState, :Value) >> + change_state >> end >> + end >> >> + def change_state >> + status = Yast::Service.Enabled("apparmor") >> + # If the service is the same state as our status, return >> + if status == @service_enabled >> + return >> + end >> >> + # Change the state to what we have >> + if @service_enabled >> + Yast::Service.start("apparmor") >> + Yast::Service.enable("apparmor") >> + else >> + Yast::Service.stop("apparmor") >> + Yast::Service.disable("apparmor") >> + end >> >> - button >> + # Check if the change of service state worked >> + status = Yast::Service.Enabled("apparmor") > > > Just note. It is quite uncommon in yast to react immediatelly in YaST UI. > Especially consider that user click on "Enable" checkbox and then press > cancel. > This should be done only after click on next, where he confirm that he really > want > to do actions. I understand. However, I just followed the design how it exists now. While I am open to changing the design, it may take me much longer to learn and implement it. Let's keep it in the next round of changes, but as a guideline, I would like to have suggestions on how it should look like. How about yast-firewall? We will have to implement a separate "Manage profiles". There are more changes I want to introduce there as well such as multiple/different toggle modes. > > >> + if status != @service_enabled >> + Yast::Report.Error(_('Failed to change apparmor service. Please use >> journal (journalctl -n -u apparmor) to diagnose')) >> + else >> + # Enable the configuration frame since everything went well >> + Yast::UI.ChangeWidget(Id(:aaEnableFrame), :Enabled, status) >> + end >> end >> + >> end >> end >> >> -Yast::ApparmorSettingsClient.new.main >> +AppArmor::ApparmorSettings.new.run > > Btw, recent practive in yast is to have really small client ( like two lines > ``` > require "y2apparmor/clients/my_client" > > Y2Apparmor::Clients::MyClient.run > ``` > > and having code for client in lib/y2apparmor/clients/my_client.rb. > This will allow much easier unit testing of code in client ( as require do > not run code automatic ) Yes, I noticed that in the yast-journalctl tutorial. The way I divided it is to keep all yast-apparmor interactions in lib/ and keep the rest in clients. Does it have to be in lib/y2apparmor/clients or just lib/ would do as yast-journalctl tutorial mentions? -- Goldwyn -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
