Refactor add on auto client #54

dgdavid · 2018-06-21T06:21:39Z

This PR is a refactorization of auto add-on client, following the approach suggested by @imobachgs and @jreidinger in #52 and discarding the first idea about refactoring only the #write method.

Help is needed to finish it, especially with the test which probably needs improve a lot. Also, help with naming and documentation is welcome.

I will do a self-review to write down some doubts in place.

dgdavid · 2018-06-21T06:30:20Z

src/lib/add-on/clients/add-on_auto.rb

-            if (srcid == -1 || srcid == nil)
-              # revert back to the unexpanded URL to have the original URL
-              # in the saved /etc/zypp/repos.d file
-              Pkg.SourceChangeUrl(srcid, media)


Reading the source code of Pkg.SourceChangeUrl, I think that this line was doing nothing. Am I right?

Yes, that was wrong.

jreidinger · 2018-06-21T06:33:20Z

src/lib/add-on/clients/add-on_auto.rb

        end
+
+        reject


why not having something like this in block?

next false unless add_on.fetch("media_url", "").empty? log.error "Missing <media_url> value in the #{index}. add-on-product definition" # abort import/installation return false unless skip_add_on_and_continue?(index) true

Oh, nice!

Thank you!

dgdavid · 2018-06-21T06:34:03Z

src/lib/add-on/clients/add-on_auto.rb

-              Pkg.SourceChangeUrl(srcid, media)
-
-              if Ops.get_boolean(prod, "ask_on_error", false)
-                prod["ask_on_error"] = Popup.ContinueCancel(


I avoided to overwrite the original value of ask_on_error property, but I have no clear if there is some reason to do it.

jreidinger · 2018-06-21T06:35:28Z

src/lib/add-on/clients/add-on_auto.rb

+        "</li>"
+      end
+
+      ["<ul>", formatted_add_on, "</ul>"].flatten.join("\n")


you can write it without flatten like ["<ul>", *formatted_add_on, "</ul>"].join("\n")

jreidinger · 2018-06-21T06:36:32Z

src/lib/add-on/clients/add-on_auto.rb

+    def reset
+      AddOnProduct.add_on_products = []
+
+      {}


not needed. Reset return value should not be used see https://github.com/yast/yast-yast2/blob/0d1f6001e99734c8da163d78c04bf53b029ba8c4/library/general/src/lib/installation/auto_client.rb#L114

jreidinger · 2018-06-21T06:37:22Z

src/lib/add-on/clients/add-on_auto.rb

+        false
+      )
+
+      UI.CloseDialog


this looks wrong for me Opening with Wizard.CreateDialog and closing with UI.CloseDialog. Why not Wizard.CloseDialog?

Honestly... I don't know :( It is just legacy code, shall we change it? (I need to learn more about "UI" and "Wizard", I guess)

jreidinger · 2018-06-21T06:40:32Z

src/lib/add-on/clients/add-on_auto.rb

+    #
+    # @return [Boolean] true if given source id is valid; false otherwise
+    def valid_source?(source_id)
+      source_id != -1 && source_id != nil # rubocop:disable Style/NonNilCheck


what about ![nil, -1].include?(source_id)? Then you do not need rubocop disable.

or better use constant like

INVALID_SOURCE_IDS = [nil, -1] .... !INVALID_SOURCE_IDS.include?(source_id)

In fact, I was using your first suggestion but then I changed it for two main reasons:

It was not clear to me the use of nil value, as I said in Refactor add on auto client #54 (comment) (@lslezak already explained the reason why it must still there).

Despite to be less "ruby style", it seems to me more "simple" and readable the condition as it was.

dgdavid · 2018-06-21T06:42:04Z

src/lib/add-on/clients/add-on_auto.rb

+    #
+    # @return [Boolean] true if given source id is valid; false otherwise
+    def valid_source?(source_id)
+      source_id != -1 && source_id != nil # rubocop:disable Style/NonNilCheck


I am keeping the original condition here, but to be honest I don't enterily understand why to concern about nil if PkgFunctions::SourceCreateEx seems to always return an integer value.

Yes, it should return -1 in case of error. But in theory it might return nil if an unhandled exception in the code is raised.

Pkg:: functions usually use nil as an error value, I guess for some historical reasons it uses -1 here... I'd keep the nil check unless it really hurts

dgdavid · 2018-06-21T06:42:32Z

src/lib/add-on/clients/add-on_auto.rb

+    #
+    # @return [String] expanded url
+    def expand_url_for(add_on, media_url)
+      AddOnProduct.SetRepoUrlAlias(


Better in a single line, isn't it?

jreidinger · 2018-06-21T07:14:08Z

src/lib/add-on/clients/add-on_auto.rb

+
+      log.info("Preferred name: #{repo["name"]}")
+
+      repos[repo_idx] = repo


as ruby use reference and not copy, does it make sense this line?

and if not, then all those index fun won't be needed and code can be much simplier.

lslezak

Some suggestions

lslezak · 2018-06-21T07:03:29Z

src/lib/add-on/clients/add-on_auto.rb

-      Yast.import "PackageLock"
-      Yast.import "Installation"
-      Yast.import "String"
+# rubocop:disable Naming/FileName


Um, I cannot see any .rubocop.yml file in the project. Without it tweaking the Rubocop config is probably useless...

And I'd prefer disabling it in the Rubocop config file instead of polluting the source code. See an example here.

You're right. I took borrowed the .rubocop.yml from yast-storage-ng to lint the code in my local copy... and then I forgot to remove those comments 😇

And I'd prefer disabling it in the Rubocop config file instead of polluting the source code.

Me too :) 👍

Well, you can add it to the package. The long term goal is to use Rubocop for all code. We just did not have time to adapt all packages.

Ok, I'll do it in its own PR :)

lslezak · 2018-06-21T07:07:21Z

src/lib/add-on/clients/add-on_auto.rb

+      # reread agents, redraw wizard steps, etc.
+      AddOnProduct.ReIntegrateFromScratch
+
+      true


I'm not sure if always returning true is a good idea. What about adding some error handling? I guess that create_source(add_on) above might fail...

Oh I see, create_source reports error internally using the report_error_for method... But I'm still not sure if always returning true is OK. If it is then it should be at least documented here that it's OK and the error is reported elsewhere.

Ok, I'll think a little about it and make some changes to improve this.

Thanks

@lslezak what about the new approach? (I just updated the PR)

OK, looks better, the report_error_for is more visible now.

lslezak · 2018-06-21T07:15:23Z

test/y2add_on/clients/add-on_auto_test.rb

@@ -3,20 +3,237 @@
 require_relative "../../test_helper"
 require "add-on/clients/add-on_auto"

+require "byebug"


Please remove this debugging require, during RPM build the byebug gem is not available.

Ouch! 🤦‍♂️ 😅

I'll remove it in next update.

lslezak · 2018-06-21T07:18:29Z

And Travis complains about a missing textdomain: https://travis-ci.org/yast/yast-add-on/builds/394877036#L381

jreidinger · 2018-06-21T07:36:00Z

src/lib/add-on/clients/add-on_auto.rb

+    #
+    # @return [String] preferred name for add-on/repo
+    def preferred_name_for(add_on, repo)
+      return add_on["name"] if add_on.key?("name") # name in control file, bnc#433981


I hope that add["name"] is enough, as for me nil value when this methods wants to return String.

What do you mean?

It should be checked that value is not empty or nil before return it?

jreidinger · 2018-06-21T07:36:13Z

src/lib/add-on/clients/add-on_auto.rb

+    # @param [Hash] product
+    # @param [Array] repo
+    # @param [String] media
+    # @param [String] pth


this looks wrong. I see two params.

jreidinger · 2018-06-21T07:36:27Z

src/lib/add-on/clients/add-on_auto.rb

+    # Returns preferred name for add-on/repo
+    #
+    # @param [Hash] product
+    # @param [Array] repo


more details will be useful here.

jreidinger · 2018-06-21T07:37:23Z

test/y2add_on/clients/add-on_auto_test.rb

@@ -3,20 +3,237 @@
 require_relative "../../test_helper"
 require "add-on/clients/add-on_auto"

+require "byebug"


ugh, this should not be in final code as we really do not want to build depend on byebug,

I forgot to remove it 😅 My fault. Thanks for catching it :)

jreidinger · 2018-06-21T07:38:19Z

test/y2add_on/clients/add-on_auto_test.rb

    before do
      allow(Yast::WFM).to receive(:Args).with(no_args).and_return([func])
      allow(Yast::WFM).to receive(:Args).with(0).and_return(func)
    end

+    context "when 'func' is 'Import'" do


what I usually did is that I believe AutoClient works and simply test directly import method, but it is up to you.

jreidinger · 2018-06-21T07:39:43Z

test/y2add_on/clients/add-on_auto_test.rb

+      end
+    end
+
+    # FIXME: delete it? probably already tested in AutoClient


yes, whole dispathing is tested in auto client. In general I (not team rule) have rule for unit tests that I assume that other units works and I test only my unit.
https://github.com/yast/yast-yast2/blob/0d1f6001e99734c8da163d78c04bf53b029ba8c4/library/general/test/auto_client_test.rb

Yes, you should only test the code implemented in your module. Testing the rest is out of scope and makes it fragile if the external code changes.

coveralls · 2018-06-21T15:50:14Z

Coverage increased (+2.3%) to 18.317% when pulling e3a196b on dgdavid:feature/refactor-add-on-auto-client into 3159a10 on yast:master.

lslezak

Some minor suggestions and fixes

lslezak · 2018-06-28T08:12:02Z

src/lib/add-on/clients/add-on_auto.rb

-            if (srcid == -1 || srcid == nil)
-              # revert back to the unexpanded URL to have the original URL
-              # in the saved /etc/zypp/repos.d file
-              Pkg.SourceChangeUrl(srcid, media)


Yes, that was wrong.

lslezak · 2018-06-28T08:26:51Z

src/lib/add-on/clients/add-on_auto.rb

+    def summary
+      formatted_add_on = AddOnProduct.add_on_products.map do |add_on|
+        "<li>" \
+          "Media: #{add_on["media_url"]}, " \


Use translated texts in the user visible summary and do not forget to HTML-escape the injected values, e.g.

# TRANSLATORS: %s is an add-on URL _("URL: %s") % CGI.escapeHTML(add_on["media_url"]) + # TRANSLATORS: %s is ... _("Path: %s") % ... +

(BTW I'd use "URL" instead of "Media", that looks good only for dvd:/// URL, "URL" is more generic.)

Hint: For larger rich text blocks (this one is not that big IMO) you might even use ERB templates, see this example. Then you can use the usual h() escaping just like in RoR...

lslezak · 2018-06-28T08:28:10Z

src/lib/add-on/clients/add-on_auto.rb

+      formatted_add_on = AddOnProduct.add_on_products.map do |add_on|
+        "<li>" \
+          "Media: #{add_on["media_url"]}, " \
+          "Path: #{add_on["product_dir"]}, " \


Maybe we could skip the path attribute if it is not defined or is empty or /. (As / is the default value.)

And the same with the product name, we should skip it if it is not defined.

lslezak · 2018-06-28T08:32:56Z

src/lib/add-on/clients/add-on_auto.rb

+
+        log.info("New source ID: #{source_id}")
+
+        if [nil, -1].include?(source_id)


A valid source id is a positive number, so maybe simpler unless id > 0 would be better here. (JFYI: src id 0 refers to the packages installed in the system, it's not a real source.)

If I understood you well previously, it is also needed to take into account the nil value returned as error by Pkg:: functions due to possible unhandled exceptions, isn't it?

(and nil > 0 will raise a NoMethodError exception)

OK, makes sense.

lslezak · 2018-06-28T08:33:17Z

src/lib/add-on/clients/add-on_auto.rb

+
+          return :report_error unless retry_on_error
+        elsif !accepted_license?(add_on, source_id)
+          # Lic


Incomplete comment?

lslezak · 2018-06-28T08:59:26Z

src/lib/add-on/clients/add-on_auto.rb

+    def preferred_name_for(add_on, repo)
+      add_on_name = add_on.fetch("name", nil)
+
+      return add_on_name unless add_on_name.nil? || add_on_name.empty? # name in control file, bnc#433981


NP: I prefer the comments above the code, trailing comments are a bit harder to read...

lslezak · 2018-06-28T09:26:55Z

src/lib/add-on/clients/add-on_auto.rb

+        "Missing mandatory <media_url> value at index %d in the <add_on_products> definition.",
+        "Skip the invalid product definition and continue with the installation?"
+      ]
+      popup_prompt = format(_(error_message.join("\n")), index)


Unfortunately this won't work, the translation won't be found by gettext. You have to use it on the string literal directly. But you can still split it to more line easily:

error_message = _( "Error in the AutoYaST <add_on> section.\n" \ "Missing mandatory <media_url> value at index %d in the <add_on_products> definition\n" \ "Skip the invalid product definition and continue with the installation?" )

lslezak · 2018-06-28T10:28:09Z

test/y2add_on/clients/add-on_auto_test.rb

+      end
+    end
+
+    context "when there are missed medie_url values in given 'add_on_products'" do


Typo, should be media_url here.

lslezak · 2018-06-28T10:34:28Z

test/y2add_on/clients/add-on_auto_test.rb

+  end
+
+  describe "#read" do
+    context "when cannot lock package" do


The module name is a bit confusing, but it's not a package lock but a package manager lock (so only one application can use it at one time).

lslezak · 2018-06-28T10:34:39Z

test/y2add_on/clients/add-on_auto_test.rb

+      end
+    end
+
+    context "when can lock package" do


The same here.

dgdavid · 2018-06-28T17:24:22Z

Guys, the PR was updated again, fixed and improved following suggestions from the code review.

I did a few extra commits splitting each group of changes made. But maybe it will be better to squash them (or those least relevant) in a "Updates from code review" or similar single commit, what do you think?

lslezak · 2018-07-23T07:59:45Z

You can use the "Squash and Merge" GitHub option below, it squashes everything to one commit before merging.

Making use of the base class Yast::Installation::Autoclient

Testing methods directly instead of do it through #run.

Use translations, escape HTML and omit useless "path" and "product" properties (empty or default values).

See #28 (comment), #54 (review), and https://bugzilla.suse.com/show_bug.cgi?id=1194851

dgdavid commented Jun 21, 2018

View reviewed changes

jreidinger reviewed Jun 21, 2018

View reviewed changes

dgdavid commented Jun 21, 2018

View reviewed changes

jreidinger reviewed Jun 21, 2018

View reviewed changes

dgdavid commented Jun 21, 2018

View reviewed changes

jreidinger reviewed Jun 21, 2018

View reviewed changes

lslezak reviewed Jun 21, 2018

View reviewed changes

jreidinger reviewed Jun 21, 2018

View reviewed changes

dgdavid force-pushed the feature/refactor-add-on-auto-client branch from b1bdb10 to 0e0e739 Compare June 21, 2018 15:47

dgdavid force-pushed the feature/refactor-add-on-auto-client branch 2 times, most recently from 1a51fb1 to 127d7fd Compare June 21, 2018 18:19

dgdavid changed the title ~~[WIP] Refactor add on auto client~~ Refactor add on auto client Jun 28, 2018

dgdavid force-pushed the feature/refactor-add-on-auto-client branch from c428e46 to c5ef642 Compare June 28, 2018 07:30

lslezak requested changes Jun 28, 2018

View reviewed changes

lslezak approved these changes Jul 23, 2018

View reviewed changes

dgdavid added 2 commits July 23, 2018 09:28

Increase test coverage for add-on auto client

e466707

Refactor add-on auto client

6327c6e

Making use of the base class Yast::Installation::Autoclient

dgdavid added 9 commits July 23, 2018 09:28

Update add_on auto client test

48d1157

Testing methods directly instead of do it through #run.

Improve Add-on auto client #summary method

c8b56ee

Use translations, escape HTML and omit useless "path" and "product" properties (empty or default values).

Fix some typos

2ddc5b4

Allow URLs written in uppercase

9fb07f9

Improve information for translators

9697564

Remove not necessaries lines of code

392e8a2

Move a comment above the code

d1e6797

Fix error message

f4e6c61

Fix typos in Add-on auto client test

e8baa4b

dgdavid force-pushed the feature/refactor-add-on-auto-client branch from 60aad9d to e8baa4b Compare July 23, 2018 08:28

Bump version and update changes file

e3a196b

dgdavid force-pushed the feature/refactor-add-on-auto-client branch from 7fe3c73 to e3a196b Compare July 23, 2018 08:47

lslezak approved these changes Jul 23, 2018

View reviewed changes

dgdavid merged commit b0d2901 into yast:master Jul 23, 2018

teclator mentioned this pull request Aug 27, 2018

Required autoyast2-installation for building the package #61

Merged

dgdavid mentioned this pull request Sep 13, 2018

Fix the repo name selection #64

Merged

dgdavid added a commit that referenced this pull request Jan 20, 2022

Restores the unexpanded repo URL

af0ce6f

See #28 (comment), #54 (review), and https://bugzilla.suse.com/show_bug.cgi?id=1194851

dgdavid mentioned this pull request Jan 20, 2022

[SLE-15-SP1] Restores unexpanded repository URL #120

Merged


		log.info("Preferred name: #{repo["name"]}")

		repos[repo_idx] = repo


		log.info("New source ID: #{source_id}")

		if [nil, -1].include?(source_id)

Refactor add on auto client #54

Refactor add on auto client #54

Conversation

dgdavid commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslezak Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslezak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslezak commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreidinger Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 21, 2018 • edited Loading

lslezak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lslezak Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgdavid Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgdavid commented Jun 28, 2018

lslezak commented Jul 23, 2018

lslezak Jun 21, 2018 •

edited

Loading

jreidinger Jun 21, 2018 •

edited

Loading

coveralls commented Jun 21, 2018 •

edited

Loading

lslezak Jun 28, 2018 •

edited

Loading

dgdavid Jun 28, 2018 •

edited

Loading