-
Notifications
You must be signed in to change notification settings - Fork 820
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
Remove generation for swagger Go code and Add static swagger codes for test #2757
Conversation
Build Failed 😱 Build Id: 6e7dce58-7aad-483b-a359-ed9dd7b4635e To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Test failed because the image
|
🤔 I'll be honest, I'm not sure this is necessary. Reason being we have the SDK conformance tests - and it's exactly the same API surface on the local binary as it is with the remote - because it's actually the same binary. What I think I'd instead recommend here is that we remove the runtime generation of the go code at test time with swagger - that way we know that if the swagger changes, the test code does not. We can keep some kind of gen target/shell script if we ever want to manually refresh the test code from the swagger spec, but we could have a static implementation and leave it there. This probably would be simpler, and require less moving parts to ensure the SDK API surface remains stable for #2644. WDYT? |
I would like to confirm your idea first.
Are you referring to this test?
Do you have an image of the test when doing it by the shell?
I understand that your idea is replacement for the current swagger test.
Is my understanding correct with your idea? |
Yep, totally makes sense to confirm first! Yes - that's 100% the test I'm thinking of 👍🏻 Compliance tests are built a little differently, but also self managed by the built toolchain. You can see some (hopefully up to date) instructions here: https://github.com/googleforgames/agones/tree/v1.26.0/build/build-sdk-images And you can see all the commands are here: My thought isn't to replace the REST API test, but instead tweak it. Right now it does the following:
What I'm suggesting is, we change it to the following:
Does that make more sense? |
Thank you your idea.
I can imagene 1 and 2. Sterp 1 & 2 are about build toolchain refactor. |
What the compliance tests do is run the local SDK server in a "test mode" where is expects a set of commands - the code you linked, is sending all the commands to the local SDK server that it expects, which will then pass/fail depending on if it gets the commands it expects. Basically because of linting and compilation failure, we put that linked Go code in a If we generate and keep the REST swagger client, then that problem should go away. Does that make more sense? |
You can see the test mode here: Line 108 in bf1b8a4
|
Thank you!
I have a Innovator Hive event at this week, so I'll update this PR at next week. |
Build Succeeded 👏 Build Id: e5bcd061-ad6d-494b-822c-70a19478ffb2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@@ -14,7 +14,6 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
mkdir /go/src/agones.dev/agones/swagger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line is bug. This line make only the empty directory.
Build Failed 😱 Build Id: 0f04b50d-283e-4bd2-a8f7-ed6ee88719e1 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
The generated swagger files failed due to lint problem. |
Build Succeeded 👏 Build Id: 875997d3-b529-4153-8a75-f0571ed1895b The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good - a few comments inline.
Given that this is primarily for testing, can we drop the generation of all the .md files, maybe also the .sh files as well? It feels like a lot of noise. WDYT?
@@ -14,7 +14,6 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
mkdir /go/src/agones.dev/agones/swagger | |||
wget -q https://repo1.maven.org/maven2/io/swagger/swagger-codegen-cli/2.4.10/swagger-codegen-cli-2.4.10.jar -O /tmp/swagger-codegen-cli.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non blocking question): Any desire to upgrade the swagger-codegen-cli ? No idea how much it matters.
Looks like this version came out in 2019
2.4.10/ 2019-11-16 16:38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to v3.0.35(latest)
@@ -0,0 +1,24 @@ | |||
# Compiled Object files, Static and Dynamic libs (Shared Objects) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? If so, it needs an Apache licence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@@ -0,0 +1,52 @@ | |||
#!/bin/sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 what's this for? Generated as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generated but unnecessary. Removed
@@ -0,0 +1,203 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this generated code will need a Apache header, and a note about it being generated.
There are some boilerplate files here: https://github.com/googleforgames/agones/tree/main/build for both go and yaml that we use for headers for generated code. Figured they might be useful.
Can't see if the swagger command line tool will do this for you, if not, you can steal from: https://github.com/googleforgames/agones/blob/main/build/build-sdk-images/go/gen.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Apache header to generated files.
I added the script which delete un-used files(*.md, *.sh, docs). And ready for review again. |
Build Succeeded 👏 Build Id: cebe3a39-11e1-479c-ab71-fc0b640e9297 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
No issues on my end - just wanted to check though, the two swagger.yaml files that are generated, do we need those, or could we clean those out too? |
Build Succeeded 👏 Build Id: beeaa50f-2b8a-4dad-8e45-a0765a044003 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
I agree with you about deleting the generated two swagger.yaml files. Updated
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: govargo, markmandel 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:
Approvers can indicate their approval by writing |
Love it! |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 2061436d-266d-4c98-9fdc-de3bb30a5b27 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
nodepools and regional clusters Updates to release checklist. (googleforgames#2772) * Updates to release checklist. Adding items that showed up in the recent release that were not written down or required better clarification. * Review updates, and some other small tweaks. Co-authored-by: Robert Bailey <[email protected]> Release 1.27.0 (googleforgames#2776) * Release 1.27.0 * Update FAQ on ExternalDNS (googleforgames#2773) The feature flag it points to have been moved to stable, so the link is not useful any more. Also removed notes on ipv6, since they aren't 100% accurate, as we were discussing in googleforgames#2767. * Updates to release checklist. (googleforgames#2772) * Updates to release checklist. Adding items that showed up in the recent release that were not written down or required better clarification. * Review updates, and some other small tweaks. Co-authored-by: Robert Bailey <[email protected]> * Release-changes * Review comment * Review changes Co-authored-by: Mark Mandel <[email protected]> Co-authored-by: Robert Bailey <[email protected]> Version updates (googleforgames#2778) Players in-game metric for when PlayerTracking is enabled (googleforgames#2765) * Check for DeletionTimestamp of fleet and gameserverset before scaling * Add metric to track player count in gameservers * check PlayerStatus is not nil * Update metrics available in docs * Wrong relref path * typo * Change name for players in game metric to player connected. Add player capacity metric. Hide docs until next agones release. * Duplicate metrics table * add gameserver player tracking metrics to fleetViews Co-authored-by: Mark Mandel <[email protected]> Remove generation for swagger Go code and Add static swagger codes for test (googleforgames#2757) Co-authored-by: Mark Mandel <[email protected]> Updated allocation yaml files under examples/ to use selectors Show how to set graceful termination in a game server that is safe to (googleforgames#2780) evict. Avoid retry from allocateFromLocalCluster under context kill. (googleforgames#2783) * Version updates * issue-2736-changes Co-authored-by: Mark Mandel <[email protected]> Bring SDK base image to debian:bullseye (googleforgames#2769) * Bring SDK base image to debian:bullseye The upgrade to gRPC solved one issue, and I also added a limit to number of processes that could run for `make -j` otherwise the whole thing would fall over (also would crash my dev machine!). Closes googleforgames#2224 * Force refresh of cpp cache on Cloud Build. * Fixes for CI: * Revert CI cache increment (don't think we need it) * Add shell to cpp image for debugging. * Fix formatting issue that is breaking CI. Co-authored-by: Robert Bailey <[email protected]> Update health-checking.md (googleforgames#2785) Fixed spell error: spec.health.failureTheshold to spec.health.failureThreshold Updated allocation yaml files under examples/ to use selectors (googleforgames#2787) Cleanup of load tests (googleforgames#2784) * issue-2744 updated changes with new description * 2744 review changes Sync Pod host ports back to GameServer in GCP (googleforgames#2782) This is the start of the implementation for googleforgames#2777: * Most of this is mechanical and implements a thin cloud product abstraction layer in pkg/cloud, instantiated with New(product). The product abstraction provides a single function so far: SyncPodPortsToGameServer. * SyncPodPortsToGameServer is inserted as a hook while syncing IP/ports, to let different cloud providers handle port allocation slightly differently (in this case, GKE Autopilot) * In GKE Autopilot, we look for a JSON string like `{"min":7000,"max":8000,"portsAssigned":{"7001":7737,"7002":7738}}` as an indication that the host ports were reassigned (per policy). As a side note to anyone watching, this is currently an unreleased feature. If we see this, we use the provided mapping to map the host ports in the GameServer.Spec. With this change, it's possible to launch a GameServer and get a healthy GameServer Pod by adding the following annotation: ``` annotations: cluster-autoscaler.kubernetes.io/safe-to-evict: "true" autopilot.gke.io/host-port-assignment: '{"min": 7000, "max": 8000}' ``` If this PR causes any issues, the cloud product auto detection can be disabled by setting `agones.cloudProduct=generic`, or forced to GKE Autopilot using `agones.cloudProduct=gke-autopilot`. In a future PR, I will add the host-port-assignment annotation automatically on Autopilot Co-authored-by: Mark Mandel <[email protected]> Update gke terraform files to allow autoscaling Fix (not really) problems reported by VSCode (googleforgames#2790) VSCode reports `main redeclared` between allocationload.go and runscenario.go due to the fact that they both look like `package main` binaries in the same directory, similar e.g. [this poster on a different project](https://stackoverflow.com/questions/66970531/vs-code-go-main-redeclared-in-this-block) To fix it, it's easy enough to just give these binaries their own package path and fix up the calling scripts. Along the way, fix a lint complaint in runscenario.go Add location variable for cluster location argument Minor fix changed default of location var to empty string GameServerRestartBeforeReadyCrash: Run serially (googleforgames#2791) Narrow the race in googleforgames#2445 by running GameServerRestartBeforeReadyCrash serially. See googleforgames#2445 (comment) for a detailed analysis. Does not fix the issue - this is stopgap until we understand how to fix it. Enable fieldalignment linter, then mostly ignore it (googleforgames#2795) Enable the fieldalignment linter by enabling all `govet` checks except shadowing. Ignore large swaths of code (tests, cmd/, APIs), and nolint'd existing complaints that seemed irrelevant. Along the way: * removed existing nolint:maligned, as `maligned` is no more. * disabled `structcheck` and `deadcode` as they are deprecated (and I think have been subsumed by other linters?) * changed `gameServerCacheEntry` to `gameServerCache`. It is the cache, not just an entry. * fixed alignment of `gameServerSetCacheEntry`. Add fswatch library to watch and batch filesystem events, use in allocator (googleforgames#2792) This pull refactors the fsnotify code in allocator/main out to a shared library, and in that shared library implements a batched notification processor. Closes googleforgames#1816: This takes a slightly different approach than specified in the issue, instead choosing to just delay processing until after a batch processing period. I chose 1s - it's far longer than necessary, but still much shorter than it takes for the secret changes to propagate to the container anyways. I considered the approach in googleforgames#1816 of trying to parse the actual events, but it's too fiddly to get exactly right: e.g. maybe you only refresh on "write", but then "chmod" could make the file readable whereas it wasn't before, "rename" could expose a file that wasn't there before, etc. Cloud product: Split port allocators, implement Autopilot port allocation/policies (googleforgames#2789) In the Agones on GKE Autopilot implementation, we have no need for the port allocator - the informer/etc. is an unnecessary moving piece. This PR allows for cloud products to provide their own port allocation implementation, and implements the GKE Autopilot "allocator". We do this by: * Splitting portallocator off to its own package. It was basically self-sufficient anyways, except it was a little too friendly with controller_test.go. I solved that by introducing a TestInterface for controller_test.go to upcast to. * Allow cloud product implementations to define their own port allocator. * Defining a new port allocator for GKE that does a simple per-port HostPort allocation, and adds the host-port-assignment annotation to the pod template. * Extend cloudproduct again to add a GameServer validator * And in Autopilot, reject if the PortPolicy is not `Dynamic` Release: Note to switch away from `agones-images` (googleforgames#2809) Since we have few guardrails on accidentally touching `agones-images` project, adding a note in the release checklist to switch back to a local development project after running a release. Flake: TestControllerGameServerCount (googleforgames#2805) Made it deterministic in the test, and got rod of the potential race conditions. Also fix it such that the util function for generating GameServer names always produce a unique name. Closes googleforgames#2804 Co-authored-by: Robert Bailey <[email protected]> Remove Windows FAQ Entry (googleforgames#2811) The contents are no longer accurate, and are covered in the installation section now. Makefile changes for adding location variable added autoscale parameters to Makefile and README Markdown fix in readme Changed LOCATION to always be set with ZONE as default use only if the variable has a value fixed extraneous characters update gke terraform exmaple module Update Node.js dependencies and package (googleforgames#2815) * Update all dependencies and Node,js to LTS version * Update other docker images that use Node.js Added autoscale to example cluster and added to website docs Added defaults and feature expiry Remove zone from gke/variable.tf file.
What type of PR is this?
/kind feature
/kind testing
What this PR does / Why we need it:
This PR changes conformance test behaviour for REST API SDK.
Currently, REST API SDK test does the following steps.
With above steps, if the developer changes the swagger definition file, the test will success because step 2's generation code will be also executed.
So we cannot detect whether the changes of the swagger files cause the breaking or not.
In this PR, I change the test steps.
After this PR, the developer who changes the swagger definition file must execute the
make run-sdk-command COMMAND=gen SDK_FOLDER=restapi
Which issue(s) this PR fixes:
Related: #2644
Special notes for your reviewer:
None