From c2d742cd72de2df487347feb9b63592b1db6b1cf Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Sun, 23 Feb 2020 16:46:14 -0800 Subject: [PATCH] Implement Alpha field best practices (#1348) Best practices outlined in https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions Closes #1347 --- pkg/apis/agones/v1/gameserver.go | 37 ++++--- pkg/apis/agones/v1/gameserver_test.go | 50 +++++++-- pkg/apis/agones/v1/zz_generated.deepcopy.go | 46 ++------ pkg/client/clientset/versioned/clientset.go | 32 ------ .../versioned/fake/clientset_generated.go | 20 ---- pkg/util/runtime/features.go | 2 +- .../Reference/agones_crd_api_reference.html | 101 ++++-------------- 7 files changed, 99 insertions(+), 189 deletions(-) diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go index e24c0820f8..f6236e989b 100644 --- a/pkg/apis/agones/v1/gameserver.go +++ b/pkg/apis/agones/v1/gameserver.go @@ -151,13 +151,9 @@ type GameServerSpec struct { SdkServer SdkServer `json:"sdkServer,omitempty"` // Template describes the Pod that will be created for the GameServer Template corev1.PodTemplateSpec `json:"template"` - // Alpha describes the alpha properties for the GameServer. - Alpha AlphaSpec `json:"alpha,omitempty"` -} - -// AlphaSpec contains the alpha properties of the GameServer. -type AlphaSpec struct { - Players PlayersSpec `json:"players"` + // (Alpha, PlayerTracking feature flag) Players provides the configuration for player tracking features. + // +optional + Players *PlayersSpec `json:"players,omitempty"` } // PlayersSpec tracks the initial player capacity, and what webhooks to send events to when there are @@ -225,7 +221,9 @@ type GameServerStatus struct { Address string `json:"address"` NodeName string `json:"nodeName"` ReservedUntil *metav1.Time `json:"reservedUntil"` - Alpha AlphaStatus `json:"alpha"` + // (Alpha, PlayerTracking feature flag) + // +optional + Players *PlayerStatus `json:"players"` } // GameServerStatusPort shows the port that was allocated to a @@ -235,11 +233,6 @@ type GameServerStatusPort struct { Port int32 `json:"port"` } -// AlphaStatus is the alpha status values for a GameServer -type AlphaStatus struct { - Players PlayerStatus `json:"players"` -} - // PlayerStatus stores the current player capacity values type PlayerStatus struct { Count int64 `json:"count"` @@ -315,7 +308,12 @@ func (gs *GameServer) applyStatusDefaults() { } if runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { - gs.Status.Alpha.Players.Capacity = gs.Spec.Alpha.Players.InitialCapacity + if gs.Spec.Players != nil { + if gs.Status.Players == nil { + gs.Status.Players = &PlayerStatus{} + } + gs.Status.Players.Capacity = gs.Spec.Players.InitialCapacity + } } } @@ -345,6 +343,17 @@ func (gss *GameServerSpec) applySchedulingDefaults() { // the returned array func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bool) { var causes []metav1.StatusCause + + if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { + if gss.Players != nil { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldValueNotSupported, + Field: "players", + Message: fmt.Sprintf("Value cannot be set unless feature flag %s is enabled,", runtime.FeaturePlayerTracking), + }) + } + } + if devAddress != "" { // verify that the value is a valid IP address. if net.ParseIP(devAddress) == nil { diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go index b99e0965f1..3ab0d8d164 100644 --- a/pkg/apis/agones/v1/gameserver_test.go +++ b/pkg/apis/agones/v1/gameserver_test.go @@ -62,6 +62,8 @@ func TestGameServerFindGameServerContainer(t *testing.T) { func TestGameServerApplyDefaults(t *testing.T) { t.Parallel() + ten := int64(10) + type expected struct { protocol corev1.Protocol state GameServerState @@ -69,7 +71,7 @@ func TestGameServerApplyDefaults(t *testing.T) { health Health scheduling apis.SchedulingStrategy sdkServer SdkServer - alphaPlayerCapacity int64 + alphaPlayerCapacity *int64 } data := map[string]struct { gameServer GameServer @@ -78,11 +80,11 @@ func TestGameServerApplyDefaults(t *testing.T) { expected expected }{ "set basic defaults on a very simple gameserver": { - featureFlags: runtime.FeaturePlayerTracking + "=true", + featureFlags: string(runtime.FeaturePlayerTracking) + "=true", gameServer: GameServer{ Spec: GameServerSpec{ - Alpha: AlphaSpec{Players: PlayersSpec{InitialCapacity: 10}}, - Ports: []GameServerPort{{ContainerPort: 999}}, + Players: &PlayersSpec{InitialCapacity: 10}, + Ports: []GameServerPort{{ContainerPort: 999}}, Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{Containers: []corev1.Container{ {Name: "testing", Image: "testing/image"}, @@ -105,7 +107,7 @@ func TestGameServerApplyDefaults(t *testing.T) { GRPCPort: 9357, HTTPPort: 9358, }, - alphaPlayerCapacity: 10, + alphaPlayerCapacity: &ten, }, }, "defaults on passthrough": { @@ -186,7 +188,6 @@ func TestGameServerApplyDefaults(t *testing.T) { gameServer: GameServer{ Spec: GameServerSpec{ Ports: []GameServerPort{{PortPolicy: Static}}, - Alpha: AlphaSpec{Players: PlayersSpec{InitialCapacity: 10}}, Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, }, @@ -346,7 +347,12 @@ func TestGameServerApplyDefaults(t *testing.T) { assert.Equal(t, test.expected.scheduling, test.gameServer.Spec.Scheduling) assert.Equal(t, test.expected.health, test.gameServer.Spec.Health) assert.Equal(t, test.expected.sdkServer, test.gameServer.Spec.SdkServer) - assert.Equal(t, test.expected.alphaPlayerCapacity, test.gameServer.Status.Alpha.Players.Capacity) + if test.expected.alphaPlayerCapacity != nil { + assert.Equal(t, *test.expected.alphaPlayerCapacity, test.gameServer.Status.Players.Capacity) + } else { + assert.Nil(t, test.gameServer.Spec.Players) + assert.Nil(t, test.gameServer.Status.Players) + } }) } } @@ -491,6 +497,36 @@ func TestGameServerValidate(t *testing.T) { assert.Len(t, causes, 2) assert.Contains(t, fields, "one.containerPort") assert.Contains(t, fields, "two.hostPort") + + t.Run("validation of "+string(runtime.FeaturePlayerTracking), func(t *testing.T) { + gs := GameServer{ + Spec: GameServerSpec{ + Players: &PlayersSpec{InitialCapacity: 10}, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, + } + gs.ApplyDefaults() + causes, ok := gs.Validate() + for _, f := range causes { + fields = append(fields, f.Field) + } + assert.False(t, ok) + assert.Len(t, causes, 1) + assert.Contains(t, fields, "players") + + err := runtime.ParseFeatures(string(runtime.FeaturePlayerTracking) + "=true") + assert.NoError(t, err) + gs = GameServer{ + Spec: GameServerSpec{ + Players: &PlayersSpec{InitialCapacity: 10}, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, + } + gs.ApplyDefaults() + causes, ok = gs.Validate() + assert.True(t, ok) + assert.Empty(t, causes) + }) } func TestGameServerPod(t *testing.T) { diff --git a/pkg/apis/agones/v1/zz_generated.deepcopy.go b/pkg/apis/agones/v1/zz_generated.deepcopy.go index bb4f9ebeea..cd571dc17f 100644 --- a/pkg/apis/agones/v1/zz_generated.deepcopy.go +++ b/pkg/apis/agones/v1/zz_generated.deepcopy.go @@ -25,40 +25,6 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *AlphaSpec) DeepCopyInto(out *AlphaSpec) { - *out = *in - in.Players.DeepCopyInto(&out.Players) - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AlphaSpec. -func (in *AlphaSpec) DeepCopy() *AlphaSpec { - if in == nil { - return nil - } - out := new(AlphaSpec) - in.DeepCopyInto(out) - return out -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *AlphaStatus) DeepCopyInto(out *AlphaStatus) { - *out = *in - out.Players = in.Players - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AlphaStatus. -func (in *AlphaStatus) DeepCopy() *AlphaStatus { - if in == nil { - return nil - } - out := new(AlphaStatus) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Fleet) DeepCopyInto(out *Fleet) { *out = *in @@ -336,7 +302,11 @@ func (in *GameServerSpec) DeepCopyInto(out *GameServerSpec) { out.Health = in.Health out.SdkServer = in.SdkServer in.Template.DeepCopyInto(&out.Template) - in.Alpha.DeepCopyInto(&out.Alpha) + if in.Players != nil { + in, out := &in.Players, &out.Players + *out = new(PlayersSpec) + (*in).DeepCopyInto(*out) + } return } @@ -362,7 +332,11 @@ func (in *GameServerStatus) DeepCopyInto(out *GameServerStatus) { in, out := &in.ReservedUntil, &out.ReservedUntil *out = (*in).DeepCopy() } - out.Alpha = in.Alpha + if in.Players != nil { + in, out := &in.Players, &out.Players + *out = new(PlayerStatus) + **out = **in + } return } diff --git a/pkg/client/clientset/versioned/clientset.go b/pkg/client/clientset/versioned/clientset.go index c6044097b0..62fb9716bd 100644 --- a/pkg/client/clientset/versioned/clientset.go +++ b/pkg/client/clientset/versioned/clientset.go @@ -31,17 +31,9 @@ import ( type Interface interface { Discovery() discovery.DiscoveryInterface AgonesV1() agonesv1.AgonesV1Interface - // Deprecated: please explicitly pick a version if possible. - Agones() agonesv1.AgonesV1Interface AllocationV1() allocationv1.AllocationV1Interface - // Deprecated: please explicitly pick a version if possible. - Allocation() allocationv1.AllocationV1Interface AutoscalingV1() autoscalingv1.AutoscalingV1Interface - // Deprecated: please explicitly pick a version if possible. - Autoscaling() autoscalingv1.AutoscalingV1Interface MulticlusterV1() multiclusterv1.MulticlusterV1Interface - // Deprecated: please explicitly pick a version if possible. - Multicluster() multiclusterv1.MulticlusterV1Interface } // Clientset contains the clients for groups. Each group has exactly one @@ -59,45 +51,21 @@ func (c *Clientset) AgonesV1() agonesv1.AgonesV1Interface { return c.agonesV1 } -// Deprecated: Agones retrieves the default version of AgonesClient. -// Please explicitly pick a version. -func (c *Clientset) Agones() agonesv1.AgonesV1Interface { - return c.agonesV1 -} - // AllocationV1 retrieves the AllocationV1Client func (c *Clientset) AllocationV1() allocationv1.AllocationV1Interface { return c.allocationV1 } -// Deprecated: Allocation retrieves the default version of AllocationClient. -// Please explicitly pick a version. -func (c *Clientset) Allocation() allocationv1.AllocationV1Interface { - return c.allocationV1 -} - // AutoscalingV1 retrieves the AutoscalingV1Client func (c *Clientset) AutoscalingV1() autoscalingv1.AutoscalingV1Interface { return c.autoscalingV1 } -// Deprecated: Autoscaling retrieves the default version of AutoscalingClient. -// Please explicitly pick a version. -func (c *Clientset) Autoscaling() autoscalingv1.AutoscalingV1Interface { - return c.autoscalingV1 -} - // MulticlusterV1 retrieves the MulticlusterV1Client func (c *Clientset) MulticlusterV1() multiclusterv1.MulticlusterV1Interface { return c.multiclusterV1 } -// Deprecated: Multicluster retrieves the default version of MulticlusterClient. -// Please explicitly pick a version. -func (c *Clientset) Multicluster() multiclusterv1.MulticlusterV1Interface { - return c.multiclusterV1 -} - // Discovery retrieves the DiscoveryClient func (c *Clientset) Discovery() discovery.DiscoveryInterface { if c == nil { diff --git a/pkg/client/clientset/versioned/fake/clientset_generated.go b/pkg/client/clientset/versioned/fake/clientset_generated.go index bb3558c3ad..a9da11b774 100644 --- a/pkg/client/clientset/versioned/fake/clientset_generated.go +++ b/pkg/client/clientset/versioned/fake/clientset_generated.go @@ -82,37 +82,17 @@ func (c *Clientset) AgonesV1() agonesv1.AgonesV1Interface { return &fakeagonesv1.FakeAgonesV1{Fake: &c.Fake} } -// Agones retrieves the AgonesV1Client -func (c *Clientset) Agones() agonesv1.AgonesV1Interface { - return &fakeagonesv1.FakeAgonesV1{Fake: &c.Fake} -} - // AllocationV1 retrieves the AllocationV1Client func (c *Clientset) AllocationV1() allocationv1.AllocationV1Interface { return &fakeallocationv1.FakeAllocationV1{Fake: &c.Fake} } -// Allocation retrieves the AllocationV1Client -func (c *Clientset) Allocation() allocationv1.AllocationV1Interface { - return &fakeallocationv1.FakeAllocationV1{Fake: &c.Fake} -} - // AutoscalingV1 retrieves the AutoscalingV1Client func (c *Clientset) AutoscalingV1() autoscalingv1.AutoscalingV1Interface { return &fakeautoscalingv1.FakeAutoscalingV1{Fake: &c.Fake} } -// Autoscaling retrieves the AutoscalingV1Client -func (c *Clientset) Autoscaling() autoscalingv1.AutoscalingV1Interface { - return &fakeautoscalingv1.FakeAutoscalingV1{Fake: &c.Fake} -} - // MulticlusterV1 retrieves the MulticlusterV1Client func (c *Clientset) MulticlusterV1() multiclusterv1.MulticlusterV1Interface { return &fakemulticlusterv1.FakeMulticlusterV1{Fake: &c.Fake} } - -// Multicluster retrieves the MulticlusterV1Client -func (c *Clientset) Multicluster() multiclusterv1.MulticlusterV1Interface { - return &fakemulticlusterv1.FakeMulticlusterV1{Fake: &c.Fake} -} diff --git a/pkg/util/runtime/features.go b/pkg/util/runtime/features.go index ed227515b1..d015c5e83d 100644 --- a/pkg/util/runtime/features.go +++ b/pkg/util/runtime/features.go @@ -31,7 +31,7 @@ const ( FeatureExample Feature = "Example" // FeaturePlayerTracking is a feature flag to enable/disable player tracking features. - FeaturePlayerTracking = "PlayerTracking" + FeaturePlayerTracking Feature = "PlayerTracking" ) var ( diff --git a/site/content/en/docs/Reference/agones_crd_api_reference.html b/site/content/en/docs/Reference/agones_crd_api_reference.html index c8fd1861cd..b9cea4d275 100644 --- a/site/content/en/docs/Reference/agones_crd_api_reference.html +++ b/site/content/en/docs/Reference/agones_crd_api_reference.html @@ -2536,15 +2536,16 @@

GameServer -alpha
+players
- -AlphaSpec + +PlayersSpec -

Alpha describes the alpha properties for the GameServer.

+(Optional) +

(Alpha, PlayerTracking feature flag) Players provides the configuration for player tracking features.

@@ -2675,68 +2676,6 @@

GameServerSet -

AlphaSpec -

-

-(Appears on: -GameServerSpec) -

-

-

AlphaSpec contains the alpha properties of the GameServer.

-

- - - - - - - - - - - - - -
FieldDescription
-players
- - -PlayersSpec - - -		 -
-

AlphaStatus -

-

-(Appears on: -GameServerStatus) -

-

-

AlphaStatus is the alpha status values for a GameServer

-

- - - - - - - - - - - - - -
FieldDescription
-players
- - -PlayerStatus - - -		 -

FleetSpec

@@ -3175,15 +3114,16 @@

GameServerSpec -alpha
+players
- -AlphaSpec + +PlayersSpec -

Alpha describes the alpha properties for the GameServer.

+(Optional) +

(Alpha, PlayerTracking feature flag) Players provides the configuration for player tracking features.

@@ -3273,14 +3213,16 @@

GameServerStatus -alpha
+players
- -AlphaStatus + +PlayerStatus +(Optional) +

(Alpha, PlayerTracking feature flag)

@@ -3448,15 +3390,16 @@

GameServerTemplateSpec -alpha
+players
- -AlphaSpec + +PlayersSpec -

Alpha describes the alpha properties for the GameServer.

+(Optional) +

(Alpha, PlayerTracking feature flag) Players provides the configuration for player tracking features.

@@ -3531,7 +3474,7 @@

PlayerStatus

(Appears on: -AlphaStatus) +GameServerStatus)

PlayerStatus stores the current player capacity values

@@ -3570,7 +3513,7 @@

PlayersSpec

(Appears on: -AlphaSpec) +GameServerSpec)

PlayersSpec tracks the initial player capacity, and what webhooks to send events to when there are