Enable RO proposal in case of error #483

mchf · 2016-12-19T13:26:01Z

https://trello.com/c/sEVjWKcJ/780-3-casp-read-only-software-selection

Proposals in control file currently have format:

<proposal_module>
  <name>software</name>
  <read_only config:type="boolean">true</read_only>
</proposal_module>

this PR changes this to

<proposal_module>
  <name>software</name>
  <!-- soft means that proposal becomes active when an error happens,
         hard means that proposal is always RO -->
  <read_only>soft</read_only>
</proposal_module>

lslezak · 2016-12-19T13:36:03Z

src/lib/installation/proposal_store.rb

+          @read_only_proposals[:hard] << name
+        when "soft"
+          @read_only_proposals[:soft] << name
+        end


The else branch is missing, it should at least log the unknown value. It would be better to put it in one of the list otherwise it will be completely lost. Probably the :soft is a good default as it allows user to fix a possible issue without locking him out.

lslezak · 2016-12-19T13:37:45Z

src/lib/installation/proposal_store.rb

-    def read_only?(client)
-      read_only_proposals.include?(client)
+    # @return [Boolean] if the client is marked as "hard" read only
+    def read_only_no_recovery?(client)


I think hard_read_only? and soft_read_only? names would sound better and better match the description.

lslezak · 2016-12-19T13:38:47Z

package/yast2-installation.changes

+Mon Dec 19 13:23:12 UTC 2016 - mfilka@suse.com
+
+- Made user's interaction possible in case of error in read-only
+  proposal.


Add a FATE reference.

mvidner · 2016-12-19T13:39:30Z

This only makes sense when other pieces of the puzzle are present, but there are no references to them

bnc/fate
trello
other PRs

lslezak · 2016-12-19T13:39:59Z

src/lib/installation/proposal_store.rb

+    #
+    # @param [String] client
+    # @return [Boolean] if the client is marked as "hard" read only
+    def read_only_no_recovery?(client)


I think hard_read_only? and soft_read_only? names would sound better and better match the description.

mvidner · 2016-12-19T13:41:57Z

In particular, I am missing the logic that says: "an error has in fact occurred so let's make a read-only part modifiable now".

lslezak · 2016-12-19T13:44:39Z

src/lib/installation/proposal_store.rb

+
+        case proposal["read_only"]
+        when "hard"
+          @read_only_proposals[:hard] << name


There is an inconsistency with the PR description, it lists <read_only>soft</read_only> config option (which is a string), here in the code is :hard symbol.

Either change the code to "hard" or change the configuration to <read_only config:type="symbol">soft</read_only>.

No, it's OK. The condition uses a string.

Oh, I overlooked that, OK...

lslezak

Please add new unit tests and adjust the existing tests (they use the old boolean value), see https://github.com/yast/yast-installation/blob/SLE-12-SP2-CASP/test/proposal_store_test.rb#L566

lslezak · 2016-12-19T14:21:34Z

src/lib/installation/proposal_runner.rb

@@ -807,7 +807,7 @@ def html_header(submod)
      title = @store.title_for(submod)

      # do not add a link if the module is read-only or link is already included
-      heading = if @store.read_only?(submod) || title.include?("<a")
+      heading = if @store.hard_read_only?(submod) || title.include?("<a")


It seems it does not handle the soft read only case correctly. For soft RO the proposal should be RO, RW only when there is an error. The error state for the soft case is not handled at all here.

mchf · 2016-12-19T14:49:39Z

There is no need to adjust old testsuite. read_only? still exists and still use boolean.

lslezak · 2016-12-19T15:08:12Z

I see, then please add new test for the added functionality.

And Travis fails because of cannot load such file -- y2country/widgets (LoadError), no idea why...

lslezak · 2016-12-19T15:55:37Z

OK, this looks better, thanks!

lslezak · 2016-12-20T08:19:11Z

test/proposal_runner_test.rb

+        expect_any_instance_of(::Installation::ProposalRunner)
+          .to receive(:submod_descriptions_and_build_menu)
+          .and_return(false)
+        expect_any_instance_of(::Installation::ProposalStore)


The test is OK, but as you mock most (all?) of the ::Installation::ProposalStore functionality it would be nice to add also some tests to https://github.com/mchf/yast-installation/blob/ae795f1094d361c5db76aace03776951e6923323/test/proposal_store_test.rb#L566, esp. for the soft RO case. TIA!

lslezak · 2016-12-20T08:47:02Z

Almost there... 😉

Please add also a test with soft RO and an error state. (Check that the header is clickable and the client can be called.)

mchf · 2016-12-20T09:37:41Z

Related PR: yast/yast-installation-control#33

mchf · 2016-12-20T10:29:36Z

Related PR: yast/skelcd-control-CAASP#16

lslezak

LGTM

lslezak · 2017-01-11T09:44:18Z

test/proposal_runner_test.rb

+        }
+
+        it "makes a proposal" do
+          allow(subject)


NP: I'd a comment here that this is needed to allow the other calls than the one expected later, otherwise it looks a bit confusing...

lslezak

Sorry, Travis fails... (A Rubocop issue + 1 test failure)

mchf · 2017-01-11T10:21:09Z

ok, I'll wait for #485, rebase and then we will see.

coveralls · 2017-01-13T15:01:59Z

Changes Unknown when pulling 7351a47 on mchf:ro_sw into ** on yast:SLE-12-SP2-CASP**.

lslezak · 2017-01-13T15:22:32Z

Please remove the last commit (Use Docker at Travis), I have fixed a packaging issue in the Travis packages. It works now.

mchf · 2017-01-13T20:37:24Z

@lslezak thanks, last commit removed. However the build this time failed in jenkins (seems like an accessibility issue)

lslezak · 2017-01-16T09:08:35Z

We can ignore the Jenkins failure, it tries to build against CASP in IBS which cannot work from the public Jenkins. Moreover after switching to Docker at Travis we do not this Jenkins job anymore

mchf · 2017-01-16T09:10:31Z

Thanks for reviews

mchf added 3 commits December 19, 2016 14:12

Implemented read-only proposal with recovery

7f8f929

Updated changelog

010c568

Added documentation

59dc802

lslezak requested changes Dec 19, 2016

View reviewed changes

lslezak reviewed Dec 19, 2016

View reviewed changes

lslezak requested changes Dec 19, 2016

View reviewed changes

mchf added 3 commits December 19, 2016 15:05

Renamed read_only_* functions

9a610c6

Added logging into read_only_proposals

96062d0

Fixed changelog

ce04f5c

lslezak requested changes Dec 19, 2016

View reviewed changes

Made soft read only working.

938c345

Added documentation

22e581a

lslezak requested changes Dec 20, 2016

View reviewed changes

Updated testsuite

5a1962a

mchf force-pushed the ro_sw branch from ae795f1 to 5a1962a Compare December 20, 2016 08:36

lslezak approved these changes Jan 11, 2017

View reviewed changes

lslezak requested changes Jan 11, 2017

View reviewed changes

Updated testsuite

4b949b8

mchf force-pushed the ro_sw branch from 34793b5 to 4b949b8 Compare January 11, 2017 10:03

mchf force-pushed the ro_sw branch from 7351a47 to 4b949b8 Compare January 13, 2017 19:58

lslezak approved these changes Jan 16, 2017

View reviewed changes

mchf merged commit 3a552da into yast:SLE-12-SP2-CASP Jan 16, 2017

imobachgs mentioned this pull request Jan 16, 2017

Add minimalistic libzypp config master #490

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable RO proposal in case of error #483

Enable RO proposal in case of error #483

mchf commented Dec 19, 2016 •

edited by kobliha

Loading

lslezak Dec 19, 2016

lslezak Dec 19, 2016

lslezak Dec 19, 2016

mvidner commented Dec 19, 2016

lslezak Dec 19, 2016

mvidner commented Dec 19, 2016

lslezak Dec 19, 2016

mvidner Dec 19, 2016

lslezak Dec 19, 2016

lslezak left a comment

lslezak Dec 19, 2016

mchf commented Dec 19, 2016

lslezak commented Dec 19, 2016

lslezak commented Dec 19, 2016

lslezak Dec 20, 2016

lslezak commented Dec 20, 2016 •

edited

Loading

mchf commented Dec 20, 2016

mchf commented Dec 20, 2016

lslezak left a comment

lslezak Jan 11, 2017

lslezak left a comment

mchf commented Jan 11, 2017

coveralls commented Jan 13, 2017

lslezak commented Jan 13, 2017

mchf commented Jan 13, 2017

lslezak commented Jan 16, 2017

mchf commented Jan 16, 2017

Enable RO proposal in case of error #483

Enable RO proposal in case of error #483

Conversation

mchf commented Dec 19, 2016 • edited by kobliha Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvidner commented Dec 19, 2016

Choose a reason for hiding this comment

mvidner commented Dec 19, 2016

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

mchf commented Dec 19, 2016

lslezak commented Dec 19, 2016

lslezak commented Dec 19, 2016

Choose a reason for hiding this comment

lslezak commented Dec 20, 2016 • edited Loading

mchf commented Dec 20, 2016

mchf commented Dec 20, 2016

lslezak left a 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

mchf commented Jan 11, 2017

coveralls commented Jan 13, 2017

lslezak commented Jan 13, 2017

mchf commented Jan 13, 2017

lslezak commented Jan 16, 2017

mchf commented Jan 16, 2017

mchf commented Dec 19, 2016 •

edited by kobliha

Loading

lslezak commented Dec 20, 2016 •

edited

Loading