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

Do not lower-case FQDN #231

Merged
merged 6 commits into from
Jul 25, 2024
Merged

Do not lower-case FQDN #231

merged 6 commits into from
Jul 25, 2024

Conversation

AndersonQ
Copy link
Member

This PR reverts #180

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.

Relates to elastic/beats#39993

@AndersonQ AndersonQ added bug Something isn't working Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Jul 23, 2024
@AndersonQ AndersonQ self-assigned this Jul 23, 2024
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.
@@ -0,0 +1,4 @@
```release-note:enhancement
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
```release-note:enhancement
```release-note:breaking-change

I would consider this to be breaking.

@jlind23
Copy link

jlind23 commented Jul 24, 2024

@AndersonQ while testing did you check that:

  • host.hostname was left untouched
  • host.name was lowercased

@AndersonQ
Copy link
Member Author

@AndersonQ while testing did you check that:

  • host.hostname was left untouched
  • host.name was lowercased

Yes, I did:

Screenshot from 2024-07-23 18-11-13

@AndersonQ
Copy link
Member Author

@andrewkroh do you think it's worth adding a flag to choose to have FQDN lower-cased or not?

andrewkroh
andrewkroh previously approved these changes Jul 25, 2024
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.

do you think it's worth adding a flag to choose to have FQDN lower-cased or not?

No, as long as this library is clear through godocs that it is leaving the value in its original format without any normalization. Going forward we'll keep the library unopinionated on hostname formats.

@@ -0,0 +1,4 @@
```release-note:breaking-change
The FQDN is returned as is, it isn't lowercased anymore. This reverts https://github.com/elastic/go-sysinfo/pull/180.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please specify the affected parts of the library. As a library consumer I would want to know that the value in types.HostInfo#Hostname and the response from FQDN()/FQDNWithContext() are affected. It makes it easier to audit if you are affected.

IMO we could entirely omit mentions of ECS. It would suffice to say that if you want to retain the old behavior that it is your responsibility to lowercase the values.

@AndersonQ AndersonQ merged commit 6d844a0 into elastic:main Jul 25, 2024
18 checks passed
@AndersonQ AndersonQ deleted the 39993-fqdn-as-is branch July 26, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants