Make Alpha & Beta CRD declaration consistent. #2982

markmandel · 2023-02-18T01:26:21Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug

/kind cleanup

/kind documentation
/kind feature
/kind hotfix

What this PR does / Why we need it:

We realised that with the CRD schema application being hidden behind a feature flag in the helm CRD yaml, then if the feature flag was turned off, an end user wouldn't get a validation error since the values passed in would be pruned by the K8s control plane.

To make thins consistent across Alpha CRD values:

Alpha & Beta schemas are always available within the CRD definitions.
Validation checks are in place for the feature flag (no change needed)
Alpha & Beta CRD values are pointers, so they can be nullable (this required a change in Eviction handling and ApiHooks interface)
Some general cleanup of comments and descriptions in the schema.

Which issue(s) this PR fixes:

Special notes for your reviewer:

pkg/apis/agones/v1/apihooksfake.go

install/helm/agones/templates/crds/_gameserverspecschema.yaml

markmandel · 2023-02-18T01:32:59Z

pkg/apis/agones/v1/gameserver.go

@@ -293,7 +293,7 @@ type GameServerStatus struct {
 	// +optional
 	Players *PlayerStatus `json:"players"`
 	// (Alpha, SafeToEvict feature flag) Eviction specifies the eviction tolerance of the GameServer.
-	Eviction Eviction `json:"eviction,omitempty"`
+	Eviction *Eviction `json:"eviction,omitempty"`


@zmerlynn just noting I made this a pointer to be consistent with all other Alpha/Beta properties added to a CRD.

(I can't find the best practice doc I originally referenced for this approach.)

Hmm, I saw that the others were but actually didn't understand why. It makes sense if they're nullable, but this field doesn't need to be nullable, I don't think.

Aha, found the reference I was looking for:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Having it be a non pointer means that if the feature flag is off, there is still some kind of data stored in etcd (probably a lot of empty strings, since that's the default). Whereas if it's a pointer, there is nothing to serialise/deserialise.

Actually, this is an optional field no -- I mean, the end user doesn't have to set it? So we should probably mark it as such.

Yes, I was just about to point out that by those conventions, we should add the +optional as well. But the concept LGTM - will give this a more thorough review in a bit (catching up).

All good - no rush. I'll add in the +optional here as well.

I approved but would like to see this before it goes in. :)

agones-bot · 2023-02-18T02:16:33Z

Build Succeeded 👏

Build Id: 1fd9f5dd-abe3-48b1-bc24-55855a00a87e

The following development artifacts have been built, and will exist for the next 30 days:

image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.30.0-a84a8d5-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.30.0-a84a8d5-linux-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.30.0-a84a8d5-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.30.0-a84a8d5-amd64
Linux C++ SDK (build): agonessdk-1.30.0-a84a8d5-amd64-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.30.0-a84a8d5-amd64.zip

A preview of the website (the last 30 builds are retained):

https://a84a8d5-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2982/head:pr_2982 && git checkout pr_2982
helm install agones ./install/helm/agones --namespace agones-system --agones.image.release=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.30.0-a84a8d5-amd64

google-oss-prow · 2023-02-22T21:12:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, zmerlynn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markmandel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

We realised that with the CRD schema application being hidden behind a feature flag in the helm CRD yaml, then if the feature flag was turned off, an end user wouldn't get a validation error since the values passed in would be pruned by the K8s control plane. To make thins consistent across Alpha CRD values: * Alpha & Beta schemas are always available within the CRD definitions. * Validation checks are in place for the feature flag (no change needed) * Alpha & Beta CRD values are pointers, so they can be nullable (this required a change in Eviction handling and ApiHooks interface) * Some general cleanup of comments and descriptions in the schema.

google-oss-prow · 2023-02-22T23:12:34Z

New changes are detected. LGTM label has been removed.

agones-bot · 2023-02-23T00:54:06Z

Build Failed 😱

Build Id: 692c3738-2383-4a5c-a0f9-8a008844d935

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

markmandel · 2023-02-23T00:59:30Z

Oh no, too many concurrent builds, never became the older PR in queue.

Finished Step #21 - "e2e-wait-to-become-leader"
ERROR
ERROR: build step 21 "gcr.io/cloud-builders/gcloud" failed: context deadline exceeded

agones-bot · 2023-02-23T03:17:27Z

Build Succeeded 👏

Build Id: b8c07c90-8488-423f-9a3b-e2f0be3888ba

The following development artifacts have been built, and will exist for the next 30 days:

image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.30.0-39f8c83-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.30.0-39f8c83-linux-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.30.0-39f8c83-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.30.0-39f8c83-amd64
Linux C++ SDK (build): agonessdk-1.30.0-39f8c83-amd64-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.30.0-39f8c83-amd64.zip

A preview of the website (the last 30 builds are retained):

https://39f8c83-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2982/head:pr_2982 && git checkout pr_2982
helm install agones ./install/helm/agones --namespace agones-system --agones.image.release=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.30.0-39f8c83-amd64

agones-bot · 2023-02-23T18:43:56Z

Build Succeeded 👏

Build Id: 3f5c0caf-078c-4d77-ab3b-149237dbdded

The following development artifacts have been built, and will exist for the next 30 days:

image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.30.0-1d10617-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.30.0-1d10617-linux-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.30.0-1d10617-amd64
image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.30.0-1d10617-amd64
Linux C++ SDK (build): agonessdk-1.30.0-1d10617-amd64-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.30.0-1d10617-amd64.zip

A preview of the website (the last 30 builds are retained):

https://1d10617-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/2982/head:pr_2982 && git checkout pr_2982
helm install agones ./install/helm/agones --namespace agones-system --agones.image.release=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.30.0-1d10617-amd64

markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Feb 18, 2023

markmandel requested review from zmerlynn, roberthbailey and gongmax February 18, 2023 01:26

google-oss-prow bot requested review from cyriltovena and EricFortin February 18, 2023 01:26

google-oss-prow bot added approved size/L labels Feb 18, 2023

markmandel commented Feb 18, 2023

View reviewed changes

roberthbailey assigned zmerlynn Feb 18, 2023

zmerlynn approved these changes Feb 22, 2023

View reviewed changes

google-oss-prow bot added the lgtm label Feb 22, 2023

markmandel force-pushed the cleanup/remove-schema-skip branch from a84a8d5 to 39f8c83 Compare February 22, 2023 23:12

google-oss-prow bot removed the lgtm label Feb 22, 2023

markmandel enabled auto-merge (squash) February 23, 2023 17:16

Merge branch 'main' into cleanup/remove-schema-skip

1d10617

markmandel merged commit ba0ddbb into googleforgames:main Feb 23, 2023

markmandel deleted the cleanup/remove-schema-skip branch February 23, 2023 18:43

Kalaiselvi84 added this to the 1.30.0 milestone Feb 28, 2023

igooch mentioned this pull request Jan 18, 2024

Request for igooch to become Approver #3600

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Alpha & Beta CRD declaration consistent. #2982

Make Alpha & Beta CRD declaration consistent. #2982

markmandel commented Feb 18, 2023

markmandel Feb 18, 2023

zmerlynn Feb 21, 2023

markmandel Feb 21, 2023

markmandel Feb 21, 2023 •

edited

Loading

zmerlynn Feb 21, 2023

markmandel Feb 21, 2023

zmerlynn Feb 22, 2023

markmandel Feb 22, 2023

agones-bot commented Feb 18, 2023

google-oss-prow bot commented Feb 22, 2023

google-oss-prow bot commented Feb 22, 2023

agones-bot commented Feb 23, 2023

markmandel commented Feb 23, 2023

agones-bot commented Feb 23, 2023

agones-bot commented Feb 23, 2023

Make Alpha & Beta CRD declaration consistent. #2982

Make Alpha & Beta CRD declaration consistent. #2982

Conversation

markmandel commented Feb 18, 2023

markmandel Feb 18, 2023

Choose a reason for hiding this comment

zmerlynn Feb 21, 2023

Choose a reason for hiding this comment

markmandel Feb 21, 2023

Choose a reason for hiding this comment

markmandel Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

zmerlynn Feb 21, 2023

Choose a reason for hiding this comment

markmandel Feb 21, 2023

Choose a reason for hiding this comment

zmerlynn Feb 22, 2023

Choose a reason for hiding this comment

markmandel Feb 22, 2023

Choose a reason for hiding this comment

agones-bot commented Feb 18, 2023

google-oss-prow bot commented Feb 22, 2023

google-oss-prow bot commented Feb 22, 2023

agones-bot commented Feb 23, 2023

markmandel commented Feb 23, 2023

agones-bot commented Feb 23, 2023

agones-bot commented Feb 23, 2023

markmandel Feb 21, 2023 •

edited

Loading