Adding convenience method for FQDN-aware hostname

[ycombinator]

What does this PR do?

This PR aliases the github.com/elastic/go-sysinfo/types/HostInfo type to github.com/elastic/elastic-agent-libs/sysinfo/HostInfo and adds a convenience method for reporting the FQDN-aware hostname.

Why is it important?

To consolidate logic of reporting the appropriate hostname in a single place.

Also see discussion on elastic/go-sysinfo#156 (comment).

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.md

Author's Checklist

• [ ]

Related issues

[elasticmachine]

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-03-27T19:41:24.235+0000

• Duration: 3 min 43 sec

Steps errors

Expand to view the steps failures

Mage check

• Took 0 min 1 sec . View more details here
• Description: mage -v check

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

[rdner]

I don't think we need this function:

1. We can access the fields directly
2. This function has 2 behaviours based on a boolean flag, I'd avoid this pattern, it's better to have 2 functions then. But as I mentioned above, those 2 functions would be just unnecessary getter functions for the accessible/public fields.

[ycombinator]

Funny, because I received exactly the opposite feedback in a very similar situation on another (related to FQDN) PR 😄:

elastic/beats#34456 (comment)

Without a helper function like this, we will repeat the (trivial) conditional logic in a few places: elastic/elastic-agent@d2c0512

This function has 2 behaviours based on a boolean flag, I'd avoid this pattern, it's better to have 2 functions then.

Can you elaborate a bit on why this pattern should be avoided? What is the downside or "cost" of such a pattern?

I think helper functions like this one are not uncommon and it's fine to have them as long as they're documented and tested well. It helps avoid repeating the same logic in multiple places and risking getting it wrong. I know the logic is fairly trivial in this case but I'm not really sure what the harm is in having a helper function encapsulate it. And, of course, the underlying public fields are still available for direct access in situations where that would make more sense.

[rdner]

Can you elaborate a bit on why this pattern should be avoided? What is the downside or "cost" of such a pattern?

sure, for example, when you see

FQDNAwareHostname(true)

you cannot tell what true means without going to the function definition. That's why boolean flags as function arguments are commonly avoided. It's better to predefine constants (like a enum) and use them instead. A constant name is way more descriptive than a boolean.

So, it could become something like:

type HostnameMode int

const (
	FQDN HostnameMode = 0
	Short HostnameMode = 1 // this might have a better name
)

func Hostname(mode HostnameMode) string

and then it can be used like:

hn := Hostname(FQDN)

If we don't want to have a type alias, it's:

func Hostname(hostInfo types.HostInfo, mode HostnameMode) string

[ycombinator]

Ah, okay. Thanks @rdner, I misunderstood your original comment. I thought the "pattern" to avoid was having helper functions like this one altogether. I see what you mean now; it's more about the ergonomics of the helper function from a readability perspective.

[andrewkroh]

I don't think it needs to alias the type to achieve the result of having a helper. You could have a standalone function that accepts a types.HostInfo as a parameter (e.g. hostname.Get(types.HostInfo, mode hostname.Mode) string).

[ycombinator]

With recent changes in how the go-sysinfo library returns FQDN, this convenience method no longer makes sense. Closing this PR without merging.