[Breaking Change] Multiple port support for GameServer#283
Merged
markmandel merged 1 commit intoagones-dev:masterfrom Jul 11, 2018
Merged
[Breaking Change] Multiple port support for GameServer#283markmandel merged 1 commit intoagones-dev:masterfrom
GameServer#283markmandel merged 1 commit intoagones-dev:masterfrom
Conversation
markmandel
added
kind/feature
Collaborator
|
Build Failed 😱 Build Id: 9cb52fe1-7640-4148-9ccd-c5c81fe9b309 Build Logs |
markmandel
force-pushed
the
feature/multiple-ports
branch
from
4327502 to
aefa4b1
Compare
Collaborator
|
Build Failed 😱 Build Id: bdf81aca-3cfc-4909-969b-f4f650e64290 Build Logs |
Collaborator
|
Build Succeeded 👏 Build Id: bf6159c2-4521-445b-ae4c-d1933abcfa22 The following development artifacts have been built, and will exist for the next 30 days:
|
markmandel
commented
Jul 10, 2018
| // we only want this to be called inside the mutex lock | ||
| // so let's define the function here so it can never be called elsewhere. | ||
| // Also the return gives an escape from the double loop | ||
| findOpenPorts := func(amount int) []pn { |
Collaborator
Author
There was a problem hiding this comment.
@enocom would love to get your take on what I'm doing here.
Contributor
There was a problem hiding this comment.
I've seen this pattern of declaring a function inside a method and then using it in the stdlib. It's something I've been meaning to do more often myself. Very nice.
cyriltovena
reviewed
Jul 11, 2018
| @@ -1,4 +1,4 @@ | |||
| # Copyright 2018 Google Inc. All Rights Reserved. | |||
| # Copyright 2017 Google Inc. All Rights Reserved. | |||
Collaborator
There was a problem hiding this comment.
Back in past, Marty !
Collaborator
Author
There was a problem hiding this comment.
Whoops. Will change.
cyriltovena
reviewed
Jul 11, 2018
| HostPort int32 `json:"hostPort,omitempty"` | ||
| // Protocol is the network protocol being used. Defaults to UDP. TCP is the only other option | ||
| Protocol corev1.Protocol `json:"protocol,omitempty"` | ||
| // GameServerPort is deprecated, to be removed in 0.4.0: |
Collaborator
There was a problem hiding this comment.
all of this could be removed no ? We have docker images, chart artifacts, github releases..., I don't think it's necessary to carry this around until 0.4, what do you think ?
Collaborator
Author
There was a problem hiding this comment.
When you say "all of this" - do you mean:
- drop the configuration backward compatibility for this release?
- Remove the comments on deprecation?
- Something else I'm not sure of?
Collaborator
There was a problem hiding this comment.
yes the two first point :)
Collaborator
Author
There was a problem hiding this comment.
I was just being nice giving people a short opportunity to migrate -- technically we are in alpha, so we can make breaking changes if need be, but I know people are actively using Agones in a couple of places, so figured I would be more gradual about breaking things 😄 ?
Do you feel strongly that we should remove it?
Collaborator
Author
There was a problem hiding this comment.
We had a chat - there was confusion around which release we were in. Looks like we're on the same page now, so merging this.
cyriltovena
reviewed
Jul 11, 2018
| // are to be exposed via the GameServer | ||
| type GameServerPort struct { | ||
| // Name is the descriptive name of the port | ||
| Name string `json:"name,omitempty"` |
Collaborator
There was a problem hiding this comment.
nice ! this looks familiar.
markmandel
force-pushed
the
feature/multiple-ports
branch
from
aefa4b1 to
2bd591a
Compare
Collaborator
|
Build Succeeded 👏 Build Id: 62325f2b-16e9-40a1-806f-d12fadd39fb9 The following development artifacts have been built, and will exist for the next 30 days:
|
This allows the `GameServer` (and anything that uses it as a template, such
as `Fleets`), to provide configuration and assignment of multiple ports to the
game server container.
For example:
```yaml
apiVersion: "stable.agones.dev/v1alpha1"
kind: GameServer
metadata:
name: "simple-udp"
spec:
ports:
- name: default
portPolicy: "dynamic"
containerPort: 7654
template:
spec:
containers:
- name: simple-udp
image: gcr.io/agones-images/udp-server:0.1
```
This also includes update to documentation - but in a separate bottom section
of each document, to allow for moving into the live version at release time.
(This is becoming a mess - a website can't come soon enough).
This also adjusts the `GameServer > Status`, to be able to handle multiple
ports. It now looks like this:
```
Status:
Address: 192.168.99.100
Node Name: agones
Ports:
Name: default
Port: 7502
State: Ready
```
While this a breaking change (due to the change in `GameServer > status`, the
previous legacy configuration of still works (even thought it's no longer
documented), as it is automatically converted to the new version.
Plans are to remove this legacy conversion functionality in 0.4.0, since being
in alpha, we have a very low commitment to backward compatibility.
As an aside - versioning for CRDs is coming soon, so this will also solve this
type of problem long term.
markmandel
force-pushed
the
feature/multiple-ports
branch
from
2bd591a to
f989694
Compare
Collaborator
|
Build Succeeded 👏 Build Id: dad947a4-ab3d-4c20-9e65-49fe8f4d8903 The following development artifacts have been built, and will exist for the next 30 days:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 allows the
GameServer(and anything that uses it as a template, such asFleets), to provide configuration and assignment of multiple ports to the game server container.For example:
This also includes update to documentation - but in a separate bottom section of each document, to allow for moving into the live version at release time. (This is becoming a mess - a website can't come soon enough).
This also adjusts the
GameServer > Status, to be able to handle multiple ports. It now looks like this:While this a breaking change (due to the change in
GameServer > status, the previous legacy configuration of still works (even thought it's no longer documented), as it is automatically converted to the new version.Plans are to remove this legacy conversion functionality in 0.4.0, since being in alpha, we have a very low commitment to backward compatibility.
As an aside - versioning for CRDs is coming soon, so this will also solve this type of problem long term.