Do not write anything when nothing was changed #402

mchf · 2016-05-11T11:42:15Z

coveralls · 2016-05-11T11:46:08Z

Coverage decreased (-0.7%) to 43.755% when pulling 62a1c6e on mchf:fate318787-dont-force-restart into de8dc4e on yast:master.

coveralls · 2016-05-11T11:48:03Z

Coverage decreased (-0.002%) to 44.446% when pulling 62a1c6e on mchf:fate318787-dont-force-restart into de8dc4e on yast:master.

coveralls · 2016-05-12T09:14:03Z

Coverage increased (+0.02%) to 44.463% when pulling 40d6a6d on mchf:fate318787-dont-force-restart into de8dc4e on yast:master.

coveralls · 2016-05-12T09:34:40Z

Coverage increased (+0.02%) to 44.472% when pulling e4aca6b on mchf:fate318787-dont-force-restart into de8dc4e on yast:master.

coveralls · 2016-05-12T11:57:11Z

Coverage increased (+0.03%) to 44.481% when pulling 333e4af on mchf:fate318787-dont-force-restart into de8dc4e on yast:master.

coveralls · 2016-05-12T18:18:49Z

Coverage increased (+0.03%) to 44.481% when pulling ab2e93e on mchf:fate318787-dont-force-restart into de8dc4e on yast:master.

nm_name variable is only assigned but never used in the code. Moreover it seems that its usage has started to be replaced since 2.15.46 (see 624c041)

Seems to be unused anywhere including installation clients

Commit always sets modified flag. Everything is written subsequently including forcing network service restart. That is too much when nothing was changed.

Which raised when querying nonexistent widget.

coveralls · 2016-06-06T12:03:56Z

Coverage decreased (-0.02%) to 45.537% when pulling e21a798 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

coveralls · 2016-06-06T13:32:05Z

Coverage decreased (-0.02%) to 45.537% when pulling 2e887a3 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

ProtectByFirewall method allways sets modified flag for firewall module. So when called unconditionally it causes rewriting of network configuration even when not needed (no zone changed).

coveralls · 2016-06-07T14:34:08Z

Coverage increased (+0.2%) to 45.799% when pulling 14ed002 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

coveralls · 2016-06-08T09:12:07Z

Coverage increased (+0.2%) to 45.783% when pulling 8e82f30 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

coveralls · 2016-06-08T09:24:25Z

Coverage increased (+0.2%) to 45.774% when pulling f35b789 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

mvidner · 2016-06-08T15:32:41Z

src/modules/LanItems.rb

@@ -2031,15 +2031,12 @@ def Commit
      # CanonicalizeIP is called to get new device map into the same shape as
      # NetworkInterfaces provides the current one.
      if current_map != NetworkInterfaces.CanonicalizeIP(new_map)
-        NetworkInterfaces.Name = Items()[@current]["ifcfg"] || ""
-        NetworkInterfaces.Current = deep_copy(newdev)
+        Items()[@current]["ifcfg"] = "" if !NetworkInterfaces.Change2(Items()[@current]["ifcfg"], newdev, false)


please assign that false to a explode_if_existing or whatever, before use

we can factor out the Items access if we use a mutating method on ::String:

ifcfg_name = Items()[@current]["ifcfg"] explode = false ifcfg_name.replace("") if !NetworkInterfaces.Change2(ifcfg_name, newdev, explode)

coveralls · 2016-06-08T16:25:15Z

Coverage increased (+0.2%) to 45.791% when pulling d2ef6e1 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

mvidner · 2016-06-13T08:11:48Z

LGTM.

The coverage has increased slightly but I've noticed that Lan.Modified, which is being changed, remains uncovered. It would be an easy win if you feel like keeping an eye on the numbers.

mchf · 2016-06-13T08:40:46Z

Thanks for hint. I'm going for another 0.001% ;-) Any small step towards 50% test coverage is appreciated ;-)

coveralls · 2016-06-13T18:05:26Z

Coverage increased (+0.2%) to 45.791% when pulling bc3ba1e on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

coveralls · 2016-06-13T18:05:29Z

Coverage increased (+0.2%) to 45.791% when pulling 75a8d87 on mchf:fate318787-dont-force-restart into a5698c2 on yast:master.

mchf · 2016-06-13T18:14:05Z

@mvidner
I've added the testsuite and changelog. Could you pls check these two updates?

mvidner · 2016-06-15T07:22:15Z

Once Lan.Modified is tested, the coverage should increase by 0.1% (7 lines of 6510). But we don't see such a change in the coveralls report.

mvidner · 2016-06-15T07:23:31Z

test/lan_test.rb

+      .to receive(method)
+      .and_return true
+
+    expect(modname.send(method)).to be true


Here you mock a method and then you call it. It does succeed but it is not testing any of production code.

mchf · 2016-06-15T07:33:53Z

That's bcs I forgot to actually run the test on Lan.Modified. Fix is on the way. Thanks

coveralls · 2016-06-15T07:38:06Z

Coverage increased (+0.3%) to 45.822% when pulling 57ccc88 on mchf:fate318787-dont-force-restart into cc8f60f on yast:master.

coveralls · 2016-06-15T08:26:25Z

Coverage increased (+0.3%) to 45.883% when pulling 0be5e01 on mchf:fate318787-dont-force-restart into cc8f60f on yast:master.

coveralls · 2016-06-15T08:33:03Z

Coverage increased (+0.3%) to 45.899% when pulling f56d2cd on mchf:fate318787-dont-force-restart into cc8f60f on yast:master.

mvidner · 2016-06-15T08:42:18Z

Yay! LGTM.

mchf · 2016-06-15T08:43:26Z

Thank you.

mchf force-pushed the fate318787-dont-force-restart branch from 21539b1 to 62a1c6e Compare May 11, 2016 11:44

mchf force-pushed the fate318787-dont-force-restart branch from e4aca6b to 333e4af Compare May 12, 2016 11:52

mchf added 15 commits June 4, 2016 19:38

Dropped unused LanItems#nm_name

96c8a82

nm_name variable is only assigned but never used in the code. Moreover it seems that its usage has started to be replaced since 2.15.46 (see 624c041)

Dropped LanItems#proposal_valid

a5a2736

Seems to be unused anywhere including installation clients

Do not commit LanItems when there is no change. fate#318787

08be8cc

Commit always sets modified flag. Everything is written subsequently including forcing network service restart. That is too much when nothing was changed.

Small cleanup in LanItems#Commit

d293463

Documentation for LanItems#Commit

432e77b

Refactored LanItems#ReplaceItemUdev

42409ef

Updated testsuite for LanItems#ReplaceItemUdev

4982363

Set modification flag when udev is updated. fate#318787

b91c978

Updated testsuite for ReplaceItemUdev regarding fate#318787

e6a507d

Do not set modification flag when no real change in udev is done.

bff7016

Documentation and input params checks for LanItems#ReplaceItemUdev

2718ca6

Happy rubocop

7cf57be

Fixed UI exception.

761f386

Which raised when querying nonexistent widget.

Check even firewall module when checking modified status

fce9f59

Do not force modified flag on LanItems when not needed

4623657

mchf force-pushed the fate318787-dont-force-restart branch from ab2e93e to e21a798 Compare June 6, 2016 12:00

mchf added 3 commits June 6, 2016 15:38

Fixed setting firewall.

e84e621

ProtectByFirewall method allways sets modified flag for firewall module. So when called unconditionally it causes rewriting of network configuration even when not needed (no zone changed).

Happy rubocop

6d63c98

Testsuite for FW modifications

847b02b

mchf force-pushed the fate318787-dont-force-restart branch from 2e887a3 to 847b02b Compare June 6, 2016 14:31

Use NetworkInterfaces API instead of direct access when commiting

8e82f30

Dropped useless public access modifier

f35b789

mvidner reviewed Jun 8, 2016
View reviewed changes

Improved readability

d2ef6e1

mvidner reviewed Jun 15, 2016
View reviewed changes

mchf added 2 commits June 15, 2016 09:31

Updated testsuite for Lan#Modified

5245a07

Updated changelog

57ccc88

mchf force-pushed the fate318787-dont-force-restart branch from bc3ba1e to 57ccc88 Compare June 15, 2016 07:34

Updated testsuite for Lan#Modified. Fixed modification status mocking

f56d2cd

mchf force-pushed the fate318787-dont-force-restart branch from 0be5e01 to f56d2cd Compare June 15, 2016 08:29

mchf merged commit f85b5a5 into yast:master Jun 15, 2016

mchf deleted the fate318787-dont-force-restart branch June 15, 2016 08:47

mvidner changed the title ~~[WIP] Do not write anything when nothing was changed~~ Do not write anything when nothing was changed Jun 21, 2016

mchf restored the fate318787-dont-force-restart branch October 10, 2016 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not write anything when nothing was changed #402

Do not write anything when nothing was changed #402

mchf commented May 11, 2016 •

edited by mvidner

Loading

coveralls commented May 11, 2016

coveralls commented May 11, 2016

coveralls commented May 12, 2016

coveralls commented May 12, 2016

coveralls commented May 12, 2016

coveralls commented May 12, 2016

coveralls commented Jun 6, 2016

coveralls commented Jun 6, 2016

coveralls commented Jun 7, 2016

coveralls commented Jun 8, 2016

coveralls commented Jun 8, 2016

mvidner Jun 8, 2016

mvidner Jun 8, 2016

coveralls commented Jun 8, 2016

mvidner commented Jun 13, 2016

mchf commented Jun 13, 2016

coveralls commented Jun 13, 2016

coveralls commented Jun 13, 2016

mchf commented Jun 13, 2016 •

edited

Loading

mvidner commented Jun 15, 2016

mvidner Jun 15, 2016

mchf commented Jun 15, 2016

coveralls commented Jun 15, 2016

coveralls commented Jun 15, 2016

coveralls commented Jun 15, 2016

mvidner commented Jun 15, 2016

mchf commented Jun 15, 2016

Do not write anything when nothing was changed #402

Do not write anything when nothing was changed #402

Conversation

mchf commented May 11, 2016 • edited by mvidner Loading

coveralls commented May 11, 2016

coveralls commented May 11, 2016

coveralls commented May 12, 2016

coveralls commented May 12, 2016

coveralls commented May 12, 2016

coveralls commented May 12, 2016

coveralls commented Jun 6, 2016

coveralls commented Jun 6, 2016

coveralls commented Jun 7, 2016

coveralls commented Jun 8, 2016

coveralls commented Jun 8, 2016

mvidner Jun 8, 2016

Choose a reason for hiding this comment

mvidner Jun 8, 2016

Choose a reason for hiding this comment

coveralls commented Jun 8, 2016

mvidner commented Jun 13, 2016

mchf commented Jun 13, 2016

coveralls commented Jun 13, 2016

coveralls commented Jun 13, 2016

mchf commented Jun 13, 2016 • edited Loading

mvidner commented Jun 15, 2016

mvidner Jun 15, 2016

Choose a reason for hiding this comment

mchf commented Jun 15, 2016

coveralls commented Jun 15, 2016

coveralls commented Jun 15, 2016

coveralls commented Jun 15, 2016

mvidner commented Jun 15, 2016

mchf commented Jun 15, 2016

mchf commented May 11, 2016 •

edited by mvidner

Loading

mchf commented Jun 13, 2016 •

edited

Loading