Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Udev methods consolidation #410

Merged
merged 10 commits into from Aug 9, 2016

Conversation

Projects
None yet
4 participants
@mchf
Copy link
Member

commented Jun 15, 2016

Purpose is to consolidate all udev rule handling methods at one place and design a shining new class for handling that.

@mchf mchf force-pushed the mchf:udev_methods_consolidation branch from 1ea3476 to e760f1a Jun 15, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jun 15, 2016

Coverage Status

Coverage decreased (-0.03%) to 45.865% when pulling e760f1a on mchf:udev_methods_consolidation into a01ee05 on yast:master.

@mchf mchf force-pushed the mchf:udev_methods_consolidation branch from e760f1a to f636806 Jun 15, 2016

@coveralls

This comment has been minimized.

Copy link

commented Jun 15, 2016

Coverage Status

Coverage decreased (-0.03%) to 45.865% when pulling f636806 on mchf:udev_methods_consolidation into a01ee05 on yast:master.

#
# @return [string] value corresponding to the key or empty string
def udev_key_value(rule, key)
raise ArgumentError if rule.nil?

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jun 15, 2016

Member

it is good practice to print also which argument have which problem so something like raise ArgumentError, "rule must not be nil" if rule.nil?

def udev_key_value(rule, key)
raise ArgumentError if rule.nil?

rule.each do |tuple|

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jun 15, 2016

Member

rule indicate one rule, but you iterate over it. A bit confusing, please document what is exactly rule param

This comment has been minimized.

Copy link
@mchf

mchf Aug 8, 2016

Author Member

well it is one rule. But in present code it is represented as an array ... e.g. KERNELS=pciid, NAME="eth0" is currently represented as `["KERNELS=pciid", NAME="eth0"]``

updated_rule = Yast::LanItems.ReplaceItemUdev(
"KERNELS",
"ATTR{address}",
"xx:01:02:03:04:05"
)
expect(updated_rule).to include "ATTR{address}==\"xx:01:02:03:04:05\""
expect(updated_rule).not_to include "KERNELS"

This comment has been minimized.

Copy link
@jreidinger

jreidinger Jun 15, 2016

Member

new method is not tested ;)

@mchf mchf force-pushed the mchf:udev_methods_consolidation branch from f636806 to e9b02fa Aug 8, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 8, 2016

Coverage Status

Coverage decreased (-0.03%) to 45.975% when pulling e9b02fa on mchf:udev_methods_consolidation into 86d324b on yast:master.


SetModified() if current_rule != new_rule
Items()[@current]["udev"] = { "net" => {} } if !Items()[@current]["udev"]

This comment has been minimized.

Copy link
@teclator

teclator Aug 8, 2016

Contributor

I would add a log here maybe explaining that we are adding a new udev rule instead of replacing a new one, but this is also one the things i don't like about calling ReplateItemUdev for Items without udev rules.

@@ -45,6 +45,23 @@ def update_udev_rule_key(rule, key, value)
rule
end

# Returns a value of the particular key in the rule
#
# @return [string] value corresponding to the key or empty string

This comment has been minimized.

Copy link
@teclator

teclator Aug 8, 2016

Contributor

Document params

@mchf

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2016

ReplaceItemUdev is updating device's udev. So, updating internal structures to be capable to carry the rule is fine from my POV. I assume that when someone tries to update udev rule, then he is fine with creating new one when needed.

@teclator

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2016

What i mean is that we could add the default one in case GetItemUdevRule(@current).empty? directly

@mchf

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

Well I tried to split udev methods to basic ones and to some with additional complexity. Generating fallback / default rules belongs to those more complex in my POV. And I'm not sure if it should be generated immediately when reading udev rules or when user asks for device renaming. E.g. It requires some more investigation in udev but I wouldn't be surprised if udev "expects" (in modern linux) to each device having own udev rule - in such case we can use first approach.

@coveralls

This comment has been minimized.

Copy link

commented Aug 9, 2016

Coverage Status

Coverage decreased (-0.03%) to 45.975% when pulling 2e73f66 on mchf:udev_methods_consolidation into 86d324b on yast:master.

@teclator

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2016

So now i think is time to remove the [WIP] from it an update bump version

@mchf mchf changed the title [WIP] Udev methods consolidation Udev methods consolidation Aug 9, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 9, 2016

Coverage Status

Coverage decreased (-0.03%) to 45.975% when pulling f53400b on mchf:udev_methods_consolidation into 86d324b on yast:master.

@teclator

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2016

LGTM

@mchf mchf merged commit cac42ab into yast:master Aug 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mchf mchf deleted the mchf:udev_methods_consolidation branch Aug 9, 2016

@mchf mchf restored the mchf:udev_methods_consolidation branch Oct 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.