Creating a FleetAllocation allocated a GameServer from a Fleet #193
Conversation
This file contains 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
markmandel
commented
May 1, 2018
| @@ -169,7 +168,7 @@ func (c *Controller) syncFleet(key string) error { | |||
| return errors.Wrapf(err, "error retrieving fleet %s from namespace %s", name, namespace) | |||
| } | |||
|
|
|||
| list, err := c.listGameServerSets(fleet) | |||
| list, err := ListGameServerSetsByFleetOwner(c.gameServerSetLister, fleet) | |||
There was a problem hiding this comment.
@enocom would love to get your opinion on this refactor (implementation here)
There were several places I needed a list type function like this, and wasn't quite sure the best way to do it.
|
Build Succeeded 👏 Build Id: 44021edf-184a-4e67-9594-49089cee089c The following development artifacts have been built, and will exist for the next 30 days:
|
EricFortin
reviewed
May 1, 2018
| } | ||
|
|
||
| // Validateupdate validates when an update occurs | ||
| func (fa *FleetAllocation) Validateupdate(new *FleetAllocation) (bool, []metav1.StatusCause) { |
There was a problem hiding this comment.
Nitpick: ValidateUpdate
EricFortin
reviewed
May 1, 2018
pkg/fleetallocation/controller.go
Outdated
| ErrNoGameServerReady = errors.New("Could not find a Ready GameServer") | ||
| ) | ||
|
|
||
| // Controller is a the GameServerSet controller |
There was a problem hiding this comment.
FleetAllocation controller
|
LGTM! |
enocom
reviewed
May 1, 2018
| // ListGameServerSetsByFleetOwner lists all the GameServerSets for a given | ||
| // Fleet | ||
| func ListGameServerSetsByFleetOwner(gameServerSetLister listerv1alpha1.GameServerSetLister, f *stablev1alpha1.Fleet) ([]*stablev1alpha1.GameServerSet, error) { | ||
| list, err := gameServerSetLister.List(labels.SelectorFromSet(labels.Set{stablev1alpha1.FleetGameServerSetLabel: f.ObjectMeta.Name})) |
There was a problem hiding this comment.
Based on a cursory reading of the code, I think you could even replace the concrete type *stablev1alpha1.Fleet with a metav1.Object interface. This way, the ListGameServerSetsByFleetOwner function might enjoy an even wider usage.
There was a problem hiding this comment.
If you use the metav1.Object interface here, then the name lookup becomes f.GetName() as with the lookup below.
There was a problem hiding this comment.
So I personally wasn't concerned with the type - since in this case, I really only want Fleets to be passed in. So using a Fleet as the type signature makes sense, at least in my mind - it should be restrictive.
I guess my concern was more around passing in the Lister to the function as an argument (rather than somehow creating another data structure with methods). This felt more lightweight, but was wondering if you saw any issues there?
There was a problem hiding this comment.
I've seen that pattern in some object-oriented code, but I'm having a hard time remembering what the pattern might be called. It seems entirely reasonable to me.
There was a problem hiding this comment.
SGTM! Thanks for the sanity check.
There was a problem hiding this comment.
I think it's basically a form of double dispatch.
This implements the FleetAllocation CRD, that when created looks at the requested Fleet, and marks one of the GameServers as Allocated (if available) and returns it's value in the FleetAllocation. This also includes a quickstart and spec for reference as well. Some refactoring was included in the PR (probably should have been a seperate PR for that).
markmandel
force-pushed
the
feature/fleet-allocation
branch
from
7a4079a
to
762b537
Compare
|
Build Succeeded 👏 Build Id: f4ab7fcb-c6b8-46c2-982a-42ad32ea92b4 The following development artifacts have been built, and will exist for the next 30 days:
|
EricFortin
approved these changes
May 2, 2018
|
Build Failed 😱 Build Id: 1c628f59-78a4-42f7-b8d7-23c553eebd76 Build Logs |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This implements the FleetAllocation CRD, that when created looks at the requested Fleet, and marks one of the GameServers as Allocated (if available) and returns it's value in the FleetAllocation.
This also includes a quickstart and spec for reference as well.
Some refactoring was included in the PR (probably should have been a seperate PR for that).