MGMT-21038: Prevent from inventory set host RequestedHostname directly

[bkopilov]

Update calls from host.RequestedHostname to GetHostnameForMsg(host) to prevent empty messages and set the ID.

List all the issues related to this PR

• [ ] New Feature
• [ ] Enhancement
• [ ] Bug fix
• [ ] Tests
• [ ] Documentation
• [ ] CI/CD

What environments does this code impact?

• [ ] Automation (CI, tools, etc)
• [ ] Cloud
• [ ] Operator Managed Deployments
• [x] None

How was this code tested?

• [ ] assisted-test-infra environment
• [ ] dev-scripts environment
• [ ] Reviewer's test appreciated
• [ ] Waiting for CI to do a full test run
• [ ] Manual (Elaborate on how it was tested)
• [x] No tests needed

Checklist

• [ ] Title and description added to both, commit and PR.
• [ ] Relevant issues have been associated (see CONTRIBUTING guide)
• [ ] This change does not require a documentation update (docstring, docs, README, etc)
• [ ] Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

[openshift-ci[bot]]

Hi @bkopilov. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[pastequo]

/lgtm

[pastequo]

/ok-to-test

[codecov[bot]]

Codecov Report

:x: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 42.83%. Comparing base (109c6e6) to head (8e28e0c). :warning: Report is 18 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/bminventory/inventory.go	50.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7962      +/-   ##
==========================================
- Coverage   43.84%   42.83%   -1.02%     
==========================================
  Files         414      414              
  Lines       71843    73550    +1707     
==========================================
+ Hits        31501    31502       +1     
- Misses      37481    39188    +1707     
+ Partials     2861     2860       -1     

Files with missing lines	Coverage Δ
internal/ignition/installmanifests.go	55.42% <100.00%> (ø)
internal/bminventory/inventory.go	71.98% <50.00%> (-0.01%)	:arrow_down:

... and 2 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bkopilov]

/retest

[pastequo]

/retest

[bkopilov]

/retest-required

[pastequo]

/retest

[bkopilov]

/retest

[AlonaKaplan]

/approve

[pastequo]

/retest-required

[bkopilov]

/retest-required

[pastequo]

/retest-required

[bkopilov]

/retest

[bkopilov]

rerun

[bkopilov]

Latest retest fails on go: gotest.tools/gotestsum@latest: gotest.tools/[email protected] requires go >= 1.24.0 (running go 1.23.9; GOTOOLCHAIN=local)

[coderabbitai[bot]]

Walkthrough

Replaced direct uses of host.RequestedHostname with hostutil.GetHostnameForMsg(...) across logs, manifest generation, and tests; removed automatic population of host.RequestedHostname in customization. Added extensive tests for network aggregation and primary IP stack logic covering IPv4, IPv6, and dual‑stack scenarios, including update and validation paths.

Changes

Cohort / File(s)	Summary
Hostname retrieval & logs internal/bminventory/inventory.go, internal/ignition/installmanifests.go	Switched hostname sources to hostutil.GetHostnameForMsg(...) when logging ignition upload failures and when populating BareMetalHost HardwareDetails Hostname.
Tests: hostname assertions subsystem/cluster_test.go, subsystem/cluster_v2_test.go, subsystem/operators_test.go	Updated tests to use hostutil.GetHostnameForMsg(h) instead of h.RequestedHostname; added hostutil imports where needed.
Network/IP-family tests and logic internal/bminventory/inventory_test.go	Added many test cases for calculateHostNetworks, getPrimaryIPStack, and updatePrimaryIPStack: host network filtering, primary IP family detection, ordering rules, dual‑stack edge cases, validation errors, and feature-flag interactions.
Customization behavior internal/bminventory/inventory.go	Removed automatic population of host.RequestedHostname in customizeHost, eliminating the prior hostname-setting code path during customization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title '[MGMT-21038] Prevent from inventory set host RequestedHostname directly' clearly references the main objective of the change: preventing the inventory from directly setting the host's RequestedHostname field. The title directly relates to the core changes in the code—replacing direct use of host.RequestedHostname with calls to hostutil.GetHostnameForMsg(). The title is specific and accurately summarizes the primary change.
Description check	✅ Passed	The pull request description provides a concise summary stating 'Update calls from host.RequestedHostname to GetHostnameForMsg(host) to prevent empty messages and set the ID.' The author has completed the provided template by selecting appropriate checkboxes: marking 'Enhancement' (implicitly, though unchecked, the intent is refactoring), 'None' for environment impact, and 'No tests needed' for testing. However, the description lacks detail on the motivation, context, dependencies, and specific testing considerations that would enhance clarity. The template requires more substantive information in several sections, though the core summary is present.

✨ Finishing touches

• [ ] 📝 Generate docstrings

[!WARNING] There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[pastequo]

/ok-to-test

[pastequo]

You need to rebase your branch against upstream master

[pastequo]

/ok-to-test

[pastequo]

/lgtm

[openshift-ci-robot]

@bkopilov: This pull request references MGMT-21038 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set.

In response to this:

Update calls from host.RequestedHostname to GetHostnameForMsg(host) to prevent empty messages and set the ID.

List all the issues related to this PR

• [ ] New Feature
• [ ] Enhancement
• [ ] Bug fix
• [ ] Tests
• [ ] Documentation
• [ ] CI/CD

What environments does this code impact?

• [ ] Automation (CI, tools, etc)
• [ ] Cloud
• [ ] Operator Managed Deployments
• [x] None

How was this code tested?

• [ ] assisted-test-infra environment
• [ ] dev-scripts environment
• [ ] Reviewer's test appreciated
• [ ] Waiting for CI to do a full test run
• [ ] Manual (Elaborate on how it was tested)
• [x] No tests needed

Checklist

• [ ] Title and description added to both, commit and PR.
• [ ] Relevant issues have been associated (see CONTRIBUTING guide)
• [ ] This change does not require a documentation update (docstring, docs, README, etc)
• [ ] Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[pastequo]

/ok-to-test

[pastequo]

/test assisted-service-rhel9-acm-ds-main-on-pull-request

[rccrdpccl]

/retest

[danmanor]

/test edge-subsystem-aws

[bkopilov]

/retest

[bkopilov]

/retest

[pastequo]

/lgtm

[pastequo]

/ok-to-test