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
Builder improvements #823
Builder improvements #823
Conversation
|
||
def name_valid_characters | ||
Yast::NetworkInterfaces.ValidCharsIfcfg | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeing all those method, maybe having object for name makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this method is not correct. It is used only at one place for validating ifcfg name but it can validate even characters which are not allowed in ifcfg (respectively network device) names - see https://github.com/yast/yast-yast2/blob/master/library/types/src/modules/String.rb#L591 and https://github.com/yast/yast-yast2/blob/master/library/types/src/modules/String.rb#L42
Correct validator is a few lines above - valid_name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so is it bug in ValidCharsIfcfg? I just need to get array with chars that is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think so
target_driver = driver | ||
if target_driver.empty? | ||
target_driver = Yast::Ops.get_string(Yast::LanItems.getCurrentItem, ["hwinfo", "module"], "") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have Hwinfo class so we can relax dependency on LanItems here
Hwinfo.new(name).module
(module method is currently missing in the Hwinfo class but it is very easy to add)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.