-
Notifications
You must be signed in to change notification settings - Fork 810
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
Creating a Fleet creates a GameServerSet #174
Creating a Fleet creates a GameServerSet #174
Conversation
Build Failed 😱 Build Id: 9e08eb3d-a08b-4b9a-af63-5120d777ee87 Build Logs
|
63314db
to
26c9365
Compare
@markmandel can be the error in this line? |
@fooock - the The test failed because I didn't run |
Build Succeeded 👏 Build Id: 7137844b-5f9f-4bd1-b6cc-2e90fc4b8bb5 The following development artifacts have been built, and will exist for the next 30 days:
|
ahhh ok, thanks for the explanation @markmandel . I didn't know it 😃 |
"Created GameServerSet %s", activeGsSet.ObjectMeta.Name) | ||
|
||
} else { | ||
// for now, we're ignoring any change to the template - will handle on the next PR |
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.
You want to support multiple game server set to implement things like rolling update?
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.
Yep, exactly!
pkg/fleets/controller.go
Outdated
return errors.Wrapf(err, "error updating replicas for gameserverset for fleet %s", fleet.ObjectMeta.Name) | ||
} | ||
c.recorder.Eventf(fleet, corev1.EventTypeNormal, "ScalingGameServerSet", | ||
"Scaling GameServerSet %s to %d", gsSetCopy.ObjectMeta.Name, gsSetCopy.Spec.Replicas) |
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.
Could we add the old number too in the event so we know if it was scaling up or down?
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! Done.
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.
Other than the scaling event, LGTM!
All this commit is doing is create a GameServerSet when a Fleet is created. Also, if the Replicas in a Fleet are updated, then the owned GameServerSet's Replicas are also updated. For this commit, we are not worrying about handling updates to the Spec (e.g. new image) to the Fleet, and doing updates to a live Fleet, we are just concerned with scaling up and down. Parent ticket: googleforgames#70
26c9365
to
745bffb
Compare
Build Succeeded 👏 Build Id: 45742169-9cab-4b8e-a93f-b88fd9d6498a The following development artifacts have been built, and will exist for the next 30 days:
|
All this commit is doing is create a GameServerSet when a Fleet is created. Also, if the Replicas in
a Fleet are updated, then the owned GameServerSet's Replicas are also updated.
For this commit, we are not worrying about handling updates to the Spec (e.g. new image) to the Fleet,
and doing updates to a live Fleet, we are just concerned with scaling up and down.
Parent ticket: #70