-
Notifications
You must be signed in to change notification settings - Fork 817
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 FleetAllocation. #856
Remove FleetAllocation. #856
Conversation
Build Failed 😱 Build Id: be7ed565-a2e4-4e2f-94e3-0f1c2ae9f2fa To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 106ea47c-b8c6-4567-896b-30eb032e94d4 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: a548563c-6e8f-49f5-aa68-4a380a827246 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:
|
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.
Thanks for getting rid of the FleetAllocation.
Should we remove vendor_fixes as well?
test/e2e/main_test.go
Outdated
|
||
// utility functions | ||
|
||
func getAllocation(f *stablev1alpha1.Fleet) *allocationv1alpha1.GameServerAllocation { |
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.
should it be part of framework.go?
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.
That is a much better spot for it! Moving.
test/e2e/fleetautoscaler_test.go
Outdated
gsa := getAllocation(flt) | ||
gsa, err = framework.AgonesClient.AllocationV1alpha1().GameServerAllocations(flt.ObjectMeta.Namespace).Create(gsa) | ||
if !assert.Nil(t, err) { | ||
assert.FailNow(t, "gameserverallocation could not be created") |
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.
include err?
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.
Switch to assert.NoError
which will output the error message if it fails. NoError
is a better choice (which i didn't realise until recently).
test/e2e/fleetautoscaler_test.go
Outdated
if !assert.Nil(t, err) { | ||
assert.FailNow(t, "gameserverallocation could not be created") | ||
} | ||
assert.Equal(t, string(allocationv1alpha1.GameServerAllocationAllocated), string(gsa.Status.State)) |
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.
These 6 lines are repeated in multiple places, can you factor them out to a func call
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.
Nice. Implementing!
@@ -1,10 +1,10 @@ | |||
# Simple Allocator Service | |||
|
|||
This service provides an example of using the [Agones API](https://godoc.org/agones.dev/agones/pkg/client/clientset/versioned/typed/stable/v1alpha1) to allocate a GameServer from a Fleet, and is used in the [Create an Allocator Service (Go)](../../docs/create_allocator_service.md) tutorial. | |||
This service provides an example of using the [Agones API](https://godoc.org/agones.dev/agones/pkg/client/clientset/versioned/typed/stable/v1alpha1) to allocate a GameServer from a Fleet, and is used in the [Create an Allocator Service (Go)](https://agones.dev/site/docs/tutorials/allocator-service-go/) tutorial. | |||
|
|||
## Allocator Service |
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.
Should we get rid of this sample now that we have the proper game server allocator in place?
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.
Sounds like a plan for when the allocator is fully operational? It might be a bit premature now, WDYT?
(Insert Death Star references here 😄)
This removes Fleet Allocation implementations. Removal of FleetAllocation docs will come in a separate PR, as this one was large enough already.
42e60dc
to
b498241
Compare
Implemented fixes! Nice stuff. Re: vendor_fixes - @ilkercelikyilmaz you will need to tell us if we can remove the |
Build Failed 😱 Build Id: 39ebd0be-4fdf-4c4a-82b2-5bc8dd87ce45 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 81974d3d-6f14-46ac-b261-5460a2df45e0 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:
|
This removes Fleet Allocation implementations.
Removal of FleetAllocation docs will come in a separate PR, as this one was large enough already.