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.

Sign up for GitHub

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

Jump to bottom

Always lowercase FQDN #180

Merged
merged 8 commits into from
Jun 1, 2023
Merged

Always lowercase FQDN #180

merged 8 commits into from
Jun 1, 2023

Conversation

ycombinator
Copy link
Contributor

According to https://www.elastic.co/guide/en/ecs/current/ecs-host.html#field-host-name:

[The host.name field] can contain what hostname returns on Unix systems, the fully qualified domain name (FQDN), or a name specified by the user. The recommended value is the lowercase FQDN of the host.

Accordingly, this PR ensures that the FQDN lookup always results in a lowercased FQDN.

@ycombinator ycombinator added the Team:Elastic-Agent Label for the Agent team label May 31, 2023
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 31, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-06-01T14:05:54.379+0000

  • Duration: 5 min 19 sec

Test stats 🧪

Test Results
Failed 0
Passed 161
Skipped 2
Total 163

@andrewkroh
Copy link
Member

Do you think we should also ensure that the Hostname is normalized to lowercase?

@andrewkroh
Copy link
Member

You'll need to add a changelog file as per https://github.com/elastic/go-sysinfo/blob/main/CONTRIBUTING.md.

@ycombinator
Copy link
Contributor Author

Do you think we should also ensure that the Hostname is normalized to lowercase?

Probably. I'll add the change to this PR.

You'll need to add a changelog file as per https://github.com/elastic/go-sysinfo/blob/main/CONTRIBUTING.md.

Thanks, didn't realize this repo now has a changelog - nice!

leehinman
leehinman approved these changes May 31, 2023
View reviewed changes
Copy link

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

LGTM

andrewkroh
andrewkroh reviewed May 31, 2023
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

There is one more location to update at

go-sysinfo/providers/aix/host_aix_ppc64.go

Line 176 in 577dee5

h.info.Hostname = v

andrewkroh
andrewkroh reviewed May 31, 2023
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I think it would be helpful to users to indicate in the godocs that the values are lowercased at

go-sysinfo/types/host.go

Line 70 in 6dcfe88

Hostname string `json:"name"` // Hostname

go-sysinfo/types/host.go

Line 29 in 6dcfe88

FQDN() (string, error)

andrewkroh
andrewkroh reviewed May 31, 2023
View reviewed changes
.changelog/180.txt Outdated Show resolved Hide resolved
ycombinator and others added 2 commits May 31, 2023 17:34
@ycombinator @andrewkroh
Update .changelog/180.txt
907ec60 
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
@ycombinator
Update godoc
490a24c
andrewkroh
andrewkroh approved these changes Jun 1, 2023
View reviewed changes
types/host.go Outdated Show resolved Hide resolved
types/host.go Outdated Show resolved Hide resolved
ycombinator and others added 2 commits June 1, 2023 07:05
@ycombinator @andrewkroh
Update types/host.go
4eb448b 
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
@ycombinator @andrewkroh
Update types/host.go
13c487c 
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
@andrewkroh andrewkroh merged commit 7185883 into elastic:main Jun 1, 2023
@cmacknz cmacknz mentioned this pull request Jun 12, 2023
Closed
@ycombinator ycombinator deleted the lowercase-fqdn branch April 30, 2024 17:38
@efd6 efd6 mentioned this pull request Jun 25, 2024
Closed
@AndersonQ AndersonQ mentioned this pull request Jul 23, 2024
Closed
@AndersonQ AndersonQ mentioned this pull request Jul 23, 2024
Merged
AndersonQ added a commit that referenced this pull request Jul 25, 2024
@AndersonQ
Do not lower-case FQDN (#231)
6d844a0 
This PR reverts #180

`Host.FQDNWithContext()` and the deprecated `Host.FQDN()` now return the FQDN as is; it isn't lowercased anymore. This also affects `types.HostInfo#Hostname` which, when it's the FQDN, won't be lowercased.

Complying with ECS for `host.name` and `host.hostname`
(https://www.elastic.co/guide/en/ecs/current/ecs-host.html#field-host-name)
should not be handled by go-sysinfo. The users of go-sysinfo should
ensure compliance with ECS if necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leehinman leehinman leehinman approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

@AndersonQ AndersonQ AndersonQ approved these changes

Assignees
No one assigned
Labels
Team:Elastic-Agent Label for the Agent team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ycombinator @elasticmachine @andrewkroh @AndersonQ @leehinman