Add hostname support to Machine via GuestConfig - #1454
Merged
afritzler merged 3 commits intoMay 5, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as "Kubernetes API"
participant Poollet as "Machine Poollet"
participant Broker as "Machine Broker"
participant IRI as "IRI / protobuf"
User->>API: Create Machine (spec.guestConfig.hostname)
API->>Poollet: watch/create Machine resource
Poollet->>IRI: prepareIRIMachine (include GuestConfig if set)
Poollet->>Broker: send IRI Machine request
Broker->>IRI: convert & populate GuestConfig in IRI MachineSpec
Broker->>Broker: create IronCore Machine with GuestConfig
Broker-->>API: creation result persisted
API-->>User: creation acknowledged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/apis/compute/validation/machine_test.go (1)
577-608: LGTM!The tests correctly cover the two key validation scenarios:
- Clearing an existing hostname produces an
InvalidFielderror- Changing an existing hostname produces an
ImmutableFielderrorConsider adding test cases for:
- Invalid hostname format (e.g., underscores, uppercase, exceeds 63 chars) to verify DNS label validation
- Setting hostname on a machine that previously had
GuestConfig: nil(should be allowed)These would provide more comprehensive coverage of the hostname validation rules.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/apis/compute/validation/machine_test.go` around lines 577 - 608, Add two new Ginkgo table entries next to the existing "remove hostname if set" and "immutable hostname if set" cases in internal/apis/compute/validation/machine_test.go: (1) an "invalid hostname format" entry that supplies a Machine with a GuestConfig.Hostname containing invalid DNS label examples (underscore, uppercase, >63 chars) and expects ContainElement(InvalidField("spec.guestConfig.hostname")); (2) an "allow set hostname when previously nil" entry that uses an old Machine with Spec.GuestConfig == nil and a new Machine with GuestConfig.Hostname set and expects Not(ContainElement(ImmutableField("spec.guestConfig.hostname"))) or no validation error for that field; place these alongside the existing Entry items so they run in the same table-driven test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/apis/compute/validation/machine_test.go`:
- Around line 577-608: Add two new Ginkgo table entries next to the existing
"remove hostname if set" and "immutable hostname if set" cases in
internal/apis/compute/validation/machine_test.go: (1) an "invalid hostname
format" entry that supplies a Machine with a GuestConfig.Hostname containing
invalid DNS label examples (underscore, uppercase, >63 chars) and expects
ContainElement(InvalidField("spec.guestConfig.hostname")); (2) an "allow set
hostname when previously nil" entry that uses an old Machine with
Spec.GuestConfig == nil and a new Machine with GuestConfig.Hostname set and
expects Not(ContainElement(ImmutableField("spec.guestConfig.hostname"))) or no
validation error for that field; place these alongside the existing Entry items
so they run in the same table-driven test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7346c92-5030-40a9-91b8-ee667637c148
⛔ Files ignored due to path filters (1)
iri/apis/machine/v1alpha1/api.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
api/compute/v1alpha1/machine_types.goapi/compute/v1alpha1/zz_generated.deepcopy.gobroker/machinebroker/server/machine.gobroker/machinebroker/server/machine_create.gobroker/machinebroker/server/machine_create_test.goclient-go/applyconfigurations/compute/v1alpha1/machineguestconfig.goclient-go/applyconfigurations/compute/v1alpha1/machinespec.goclient-go/applyconfigurations/internal/internal.goclient-go/applyconfigurations/utils.goclient-go/openapi/zz_generated.openapi.gointernal/apis/compute/machine_types.gointernal/apis/compute/v1alpha1/zz_generated.conversion.gointernal/apis/compute/validation/machine.gointernal/apis/compute/validation/machine_test.gointernal/apis/compute/zz_generated.deepcopy.goiri/apis/machine/v1alpha1/api.protopoollet/machinepoollet/controllers/machine_controller.gopoollet/machinepoollet/controllers/machine_controller_test.go
ushabelgur
marked this pull request as draft
March 12, 2026 11:15
ushabelgur
marked this pull request as ready for review
March 13, 2026 09:06
lukasfrank
reviewed
Mar 20, 2026
opensovereigncloud-user
force-pushed
the
osc/enh/machine_hostname
branch
from
March 24, 2026 12:03
1c07f62 to
99ed8ae
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/apis/compute/validation/machine_test.go (1)
577-636: Tests correctly cover the update validation logic.The four test cases properly verify the hostname immutability/set-once semantics implemented in
validateMachineSpecUpdate.Consider adding test coverage for hostname format validation in the
ValidateMachinetable (lines 26-207) to verify that invalid DNS labels are rejected:Suggested additional test cases for ValidateMachine
Entry("invalid guestConfig hostname", &compute.Machine{ Spec: compute.MachineSpec{ GuestConfig: &compute.MachineGuestConfig{ Hostname: "foo*bar", }, }, }, ContainElement(InvalidField("spec.guestConfig.hostname")), ), Entry("valid guestConfig hostname", &compute.Machine{ Spec: compute.MachineSpec{ GuestConfig: &compute.MachineGuestConfig{ Hostname: "my-hostname", }, }, }, Not(ContainElement(InvalidField("spec.guestConfig.hostname"))), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/apis/compute/validation/machine_test.go` around lines 577 - 636, The ValidateMachine tests lack cases that assert hostname format validation; add table-driven tests in the ValidateMachine suite (the table around ValidateMachine) to cover invalid and valid DNS-label hostnames by creating Machine objects with Spec.GuestConfig.Hostname set to "foo*bar" (expect ContainElement(InvalidField("spec.guestConfig.hostname"))) and "my-hostname" (expect Not(ContainElement(InvalidField("spec.guestConfig.hostname")))); this ensures the ValidateMachine validation path (which complements validateMachineSpecUpdate) rejects malformed hostnames and accepts valid ones.internal/apis/compute/machine_types.go (1)
42-43: Consider removing the JSON tag for consistency with other fields.Other fields in
MachineSpec(lines 17-41) do not have JSON tags. Since this is an internal API package and serialization is typically handled through conversion to versioned types, this tag creates an inconsistency.Suggested fix
// GuestConfig contains an optional guest OS level configuration for the machine. - GuestConfig *MachineGuestConfig `json:"guestConfig,omitempty"` + GuestConfig *MachineGuestConfig🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/apis/compute/machine_types.go` around lines 42 - 43, The GuestConfig field in MachineSpec currently has an explicit json tag; remove the `json:"guestConfig,omitempty"` tag from the GuestConfig declaration so it matches the other untagged fields in MachineSpec and relies on conversion/versioned serialization; update the struct field `GuestConfig *MachineGuestConfig` accordingly to maintain consistency with the surrounding fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/apis/compute/machine_types.go`:
- Around line 42-43: The GuestConfig field in MachineSpec currently has an
explicit json tag; remove the `json:"guestConfig,omitempty"` tag from the
GuestConfig declaration so it matches the other untagged fields in MachineSpec
and relies on conversion/versioned serialization; update the struct field
`GuestConfig *MachineGuestConfig` accordingly to maintain consistency with the
surrounding fields.
In `@internal/apis/compute/validation/machine_test.go`:
- Around line 577-636: The ValidateMachine tests lack cases that assert hostname
format validation; add table-driven tests in the ValidateMachine suite (the
table around ValidateMachine) to cover invalid and valid DNS-label hostnames by
creating Machine objects with Spec.GuestConfig.Hostname set to "foo*bar" (expect
ContainElement(InvalidField("spec.guestConfig.hostname"))) and "my-hostname"
(expect Not(ContainElement(InvalidField("spec.guestConfig.hostname")))); this
ensures the ValidateMachine validation path (which complements
validateMachineSpecUpdate) rejects malformed hostnames and accepts valid ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4004b771-bec5-4f9e-8897-d1d08d447ea4
⛔ Files ignored due to path filters (1)
iri/apis/machine/v1alpha1/api.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
api/compute/v1alpha1/machine_types.goapi/compute/v1alpha1/zz_generated.deepcopy.gobroker/machinebroker/server/machine.gobroker/machinebroker/server/machine_create.gobroker/machinebroker/server/machine_create_test.goclient-go/applyconfigurations/compute/v1alpha1/machineguestconfig.goclient-go/applyconfigurations/compute/v1alpha1/machinespec.goclient-go/applyconfigurations/internal/internal.goclient-go/applyconfigurations/utils.goclient-go/openapi/zz_generated.openapi.gointernal/apis/compute/machine_types.gointernal/apis/compute/v1alpha1/zz_generated.conversion.gointernal/apis/compute/validation/machine.gointernal/apis/compute/validation/machine_test.gointernal/apis/compute/zz_generated.deepcopy.goiri/apis/machine/v1alpha1/api.protopoollet/machinepoollet/controllers/machine_controller.gopoollet/machinepoollet/controllers/machine_controller_test.go
✅ Files skipped from review due to trivial changes (3)
- client-go/applyconfigurations/utils.go
- broker/machinebroker/server/machine_create_test.go
- client-go/applyconfigurations/compute/v1alpha1/machineguestconfig.go
🚧 Files skipped from review as they are similar to previous changes (9)
- poollet/machinepoollet/controllers/machine_controller_test.go
- iri/apis/machine/v1alpha1/api.proto
- api/compute/v1alpha1/machine_types.go
- client-go/applyconfigurations/compute/v1alpha1/machinespec.go
- broker/machinebroker/server/machine_create.go
- client-go/applyconfigurations/internal/internal.go
- internal/apis/compute/zz_generated.deepcopy.go
- api/compute/v1alpha1/zz_generated.deepcopy.go
- internal/apis/compute/v1alpha1/zz_generated.conversion.go
lukasfrank
approved these changes
Mar 24, 2026
opensovereigncloud-user
force-pushed
the
osc/enh/machine_hostname
branch
from
March 26, 2026 07:34
99ed8ae to
07999aa
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/apis/compute/machine_types.go (1)
42-43: Remove inconsistent JSON tag from internal API type.The
GuestConfigfield in internalMachineSpechas an explicit JSON tagjson:"guestConfig,omitempty", but the internal API pattern (as seen inMachineClassRefand other fields) is to omit JSON tags. JSON tags belong at the versioned API layer, not in internal API types.♻️ Suggested fix
// GuestConfig contains an optional guest OS level configuration for the machine. - GuestConfig *MachineGuestConfig `json:"guestConfig,omitempty"` + GuestConfig *MachineGuestConfig🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/apis/compute/machine_types.go` around lines 42 - 43, The GuestConfig field on the internal MachineSpec currently has an explicit JSON tag (`GuestConfig *MachineGuestConfig json:"guestConfig,omitempty"`) which violates the internal API pattern; remove the JSON struct tag so the field is declared simply as `GuestConfig *MachineGuestConfig` to match other internal fields like `MachineClassRef` and ensure JSON tags are only applied in the versioned API types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/apis/compute/machine_types.go`:
- Around line 42-43: The GuestConfig field on the internal MachineSpec currently
has an explicit JSON tag (`GuestConfig *MachineGuestConfig
json:"guestConfig,omitempty"`) which violates the internal API pattern; remove
the JSON struct tag so the field is declared simply as `GuestConfig
*MachineGuestConfig` to match other internal fields like `MachineClassRef` and
ensure JSON tags are only applied in the versioned API types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0db5c5e-5f63-4a32-b89b-5eb43e69e39f
⛔ Files ignored due to path filters (1)
iri/apis/machine/v1alpha1/api.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
api/compute/v1alpha1/machine_types.goapi/compute/v1alpha1/zz_generated.deepcopy.gobroker/machinebroker/server/machine.gobroker/machinebroker/server/machine_create.gobroker/machinebroker/server/machine_create_test.goclient-go/applyconfigurations/compute/v1alpha1/machineguestconfig.goclient-go/applyconfigurations/compute/v1alpha1/machinespec.goclient-go/applyconfigurations/internal/internal.goclient-go/applyconfigurations/utils.goclient-go/openapi/zz_generated.openapi.gointernal/apis/compute/machine_types.gointernal/apis/compute/v1alpha1/zz_generated.conversion.gointernal/apis/compute/validation/machine.gointernal/apis/compute/validation/machine_test.gointernal/apis/compute/zz_generated.deepcopy.goiri/apis/machine/v1alpha1/api.protopoollet/machinepoollet/controllers/machine_controller.gopoollet/machinepoollet/controllers/machine_controller_test.go
✅ Files skipped from review due to trivial changes (4)
- broker/machinebroker/server/machine_create_test.go
- client-go/applyconfigurations/utils.go
- poollet/machinepoollet/controllers/machine_controller.go
- client-go/applyconfigurations/compute/v1alpha1/machineguestconfig.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/apis/compute/validation/machine_test.go
- internal/apis/compute/validation/machine.go
- poollet/machinepoollet/controllers/machine_controller_test.go
- iri/apis/machine/v1alpha1/api.proto
- api/compute/v1alpha1/machine_types.go
- client-go/applyconfigurations/internal/internal.go
- api/compute/v1alpha1/zz_generated.deepcopy.go
- internal/apis/compute/zz_generated.deepcopy.go
- client-go/openapi/zz_generated.openapi.go
- internal/apis/compute/v1alpha1/zz_generated.conversion.go
opensovereigncloud-user
force-pushed
the
osc/enh/machine_hostname
branch
from
April 16, 2026 07:53
07999aa to
8571e59
Compare
opensovereigncloud-user
force-pushed
the
osc/enh/machine_hostname
branch
from
April 20, 2026 08:05
8571e59 to
ea75783
Compare
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
lukasfrank
approved these changes
Apr 23, 2026
lukasfrank
changed the title
guestConfig along with hostname to Machine typehostname support to Machine via GuestConfig
Contributor
Author
|
@afritzler Can we get this merged ? |
opensovereigncloud-user
force-pushed
the
osc/enh/machine_hostname
branch
from
April 30, 2026 08:07
ea75783 to
07b83d4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
guestConfigalong withhostnametoMachinetype, which will allow users to specify a hostname for aMachinehostnamefieldFixes #1450
Summary by CodeRabbit