Mailinglist Archive: yast-devel (46 mails)

< Previous Next >
Re: [yast-devel] [PATCH 1/3] Change Apparmor settings to use UI


On 06/08/2017 02:33 AM, Josef Reidinger wrote:
On Wed, 7 Jun 2017 15:38:45 -0500
Goldwyn Rodrigues <rgoldwyn@xxxxxxx> wrote:

From: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>

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 <rgoldwyn@xxxxxxxx>
---
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: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >