Metadata propagation from fleet allocation to game server #312
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
|
Build Succeeded 👏 Build Id: 0b0486f6-b76d-4a39-b9a4-c9a8e4a0e301 The following development artifacts have been built, and will exist for the next 30 days:
|
victor-prodan
force-pushed
the
allocmeta2
branch
from
950bf87 to
9bf56a6
Compare
|
Build Succeeded 👏 Build Id: 925af27b-e5b5-4866-8bcd-588826ba617d The following development artifacts have been built, and will exist for the next 30 days:
|
victor-prodan
force-pushed
the
allocmeta2
branch
from
9bf56a6 to
ae6c16b
Compare
|
Build Failed 😱 Build Id: 95033e70-2676-4dfe-be95-63bbd244036b Build Logs |
cyriltovena
reviewed
Jul 31, 2018
| @@ -288,3 +292,25 @@ func (c *Controller) allocate(f *stablev1alpha1.Fleet) (*stablev1alpha1.GameServ | |||
|
|
|||
| return gs, nil | |||
| } | |||
|
|
|||
| // patch the labels and annotations of an allocated GameServer with metadata from a FleetAllocation | |||
| func (c *Controller) patchMetadata(gs *stablev1alpha1.GameServer, fam *stablev1alpha1.FleetAllocationMeta) { | |||
There was a problem hiding this comment.
@markmandel do you think we should move this to func(gs *stablev1alpha1.GameServer) MergeFleetMetadata(fam *stablev1alpha1.FleetAllocationMeta) and use it
if fam != nil {
gsCopy.MergeFleetMetadata(fam)
}
There was a problem hiding this comment.
Hmnnn. Interesting question.
I guess I'm more inclined to leave it - since it's functionality that is only used in this particular place. If it ends up being logic that needs to be shared, we could move it then.
Do you think it likely that we would need to have this logic be run in multiple places?
There was a problem hiding this comment.
no but it could be a function though since c is not used
There was a problem hiding this comment.
We were talking of the game server being able to update its own metadata, right?
Could this function be reused for that?
And in that case, the function should be more generic, taking individual labels and annotations maps, as the game server should not be concerned about fleet allocations...
What do you think?
There was a problem hiding this comment.
There are lots of places where the method origin is not used - I feel that's more about providing context for where the functionality is being written. At least that's my perspective. If we move things into the GameServer - I believe they should have a wider usage.
@victor-prodan that's an interesting point.
Something like a (gs *GameServer) PatchMetaData(annotations map[string]string, labels map[string]string) ?
That being said, we could also revisit this when are actually doing the work to allow metadata updates, and actually know what the requirements would be, rather than trying to guess them now. I'm more in favour of copy-pasting twice, and implementing an abstraction on the third time 😄 (or thereabouts) than trying to abstract from the outset.
Personally, I think this PR is good to go. I'm happy to give it my approval.
There was a problem hiding this comment.
Yes, that's what I had in mind but I guess it's safer to wait until we have a definite need.
|
Build Succeeded 👏 Build Id: 476d902e-4f0d-420b-8594-768d256319cb The following development artifacts have been built, and will exist for the next 30 days:
|
markmandel
added
kind/feature
|
I almost hit approve, but I realised there is but one small thing missing! The documentation for the spec needs updating as well - always be documenting! 😄 Probably just needs a section on the Although, please add a ":warning::warning::warning: This is currently a development feature and has not been released :warning::warning::warning:" above it, so that people are aware that it's not yet in a release (we remove these later). This is just something we're doing until we get a website sorted out. |
In the fleet allocation configuration you can add optional custom metadata that will be added to the game server in the moment of allocation Generated files
victor-prodan
force-pushed
the
allocmeta2
branch
from
ae6c16b to
51d550c
Compare
|
Build Succeeded 👏 Build Id: 89a3db52-5e30-465d-a97b-a6012ba28997 The following development artifacts have been built, and will exist for the next 30 days:
|
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.
In the fleet allocation configuration you can add optional custom metadata that will be added to the game server in the moment of allocation
Implements #277
Contains fixes requested in #307 (which I closed because git and I have a mutual hate relationship)