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

Sle 15 sp4 #12

Merged
merged 19 commits into from Aug 10, 2022
Merged

Sle 15 sp4 #12

merged 19 commits into from Aug 10, 2022

Conversation

varkoly
Copy link
Collaborator

@varkoly varkoly commented Aug 9, 2022

Problem

Short description of the original problem.
varkoly:master

Copy link

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few notes, but nothing blocking the merge.

@@ -53,6 +54,10 @@ class SAPHAClass < Client

def initialize
log.warn "--- called #{self.class}.#{__callee__}: CLI arguments are #{WFM.Args} ---"
#Take care that corosync is enabled and running
corosync = Yast2::Systemd::Service.find('corosync')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/yast/yast-sap-ha/pull/11/files#r942146359. But if we are pretty sure that the corosync service is there, it should not be a problem. Just take it into account if we decide at some point to change the dependency, or the service gets renamed or whatever.

@@ -235,8 +239,7 @@ def integer_in_range(value, low, high, message = '', field_name = '')
def sap_sid(value, message = '', field_name = '')
message = "A valid SAP System ID consists of three characters, starts with a letter, and "\
" must not collide with one of the reserved IDs" if message.nil? || message.empty?
flag = !SAP_SID_REGEXP.match(value).nil?
flag &= !RESERVED_SAP_SIDS.include?(value)
flag = !SAP_SID_REGEXP.match(value).nil? && !RESERVED_SAP_SIDS.include?(value)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np: same as #9 (comment).

@@ -41,15 +41,15 @@ def interfaces
# Get local machine's IPv4 addresses excluding the loopback iface
def ip_addresses
interfaces = Socket.getifaddrs.select do |iface|
iface.addr.ipv4? && !iface.addr.ipv4_loopback?
!iface.addr.nil? && iface.addr.ipv4? && !iface.addr.ipv4_loopback?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is enought to say iface.addr && iface.addr.ipv4? && !iface.addr.ipv4_loopback?.

@@ -235,8 +239,7 @@ def integer_in_range(value, low, high, message = '', field_name = '')
def sap_sid(value, message = '', field_name = '')
message = "A valid SAP System ID consists of three characters, starts with a letter, and "\
" must not collide with one of the reserved IDs" if message.nil? || message.empty?
flag = !SAP_SID_REGEXP.match(value).nil?
flag &= !RESERVED_SAP_SIDS.include?(value)
flag = !SAP_SID_REGEXP.match?(value).nil? && !RESERVED_SAP_SIDS.include?(value)
Copy link

@imobachgs imobachgs Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you decide to use match?, you can drop the ! and the nil?. See #9 (comment). If you prefer, you can leave it as it was at the beginning.

Copy link

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it looks good. Thanks!

@varkoly varkoly merged commit f4ba558 into yast:SLE-15-SP4 Aug 10, 2022
@yast-bot
Copy link

yast-bot commented Nov 9, 2022

❌ Internal Jenkins job #1 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants