-
Notifications
You must be signed in to change notification settings - Fork 681
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
Add output for port/protocol for host resource. #2202
Conversation
Signed-off-by: Jared Quick <[email protected]>
fac668b
to
e7b136e
Compare
This fixes #2085. Port and protocol are now shown in output of the host resource if defined. Signed-off-by: Jared Quick <[email protected]>
61271c3
to
29c384e
Compare
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.
This looks good to me, @jquick! My suggestion isn't a blocker but would avoid an early return. Thanks for the fix!
lib/resources/host.rb
Outdated
@@ -113,6 +113,7 @@ def ipaddress | |||
end | |||
|
|||
def to_s | |||
return "Host #{hostname} port #{port} proto #{protocol}" if port |
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 this is fine. I'd consider "building" the string instead of the early return
so we're not duplicating the "Host hostname" bit... something like this:
def to_s
resource_name = "Host #{hostname}"
resource_name += " port #{port} proto #{protocol}" if port
resource_name
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.
Sounds good, just pushed out a update with the changes. Thanks for the help!
Signed-off-by: Jared Quick <[email protected]>
8cc993b
to
4a3ef8c
Compare
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.
Looks great, and tests are all green!
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.
Thank you @jquick !! 😁
This fixes #2085. Port and protocol are now shown in output of the host resource if defined.