Skip to content
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

Lightweight library for implementing APIServer extensions #659

Merged
merged 2 commits into from
Apr 3, 2019

Conversation

markmandel
Copy link
Member

@markmandel markmandel commented Mar 16, 2019

The other API extension libraries were too opinionated, or not flexible enough to provide exactly what we need -- register a single noun in the API, not have storage, give full control over the http lifecycle.

This provides just enough functionality for what we need, while also providing extension points in case we need to expand the API surface area in the future - such as a more explict OpenAPI spec.

This is the foundational work that is required for converting GameServerAllocations into an API Extension - next step will be to use this library to implement that functionality.

@markmandel markmandel added the kind/feature New features for Agones label Mar 16, 2019
@markmandel markmandel added this to the 0.9.0 milestone Mar 16, 2019
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 8bd41393-a3f2-446c-b5cf-a6e98062233d

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/659/head:pr_659 && git checkout pr_659
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.9.0-9a9b93b

@markmandel
Copy link
Member Author

It will likely make things easier to review, but here is a (slightly out of date) example - https://github.com/markmandel/agones/blob/feature/gsa-api-server/pkg/gameserverallocations/controller.go#L104

So you can see how the pieces end up fitting together.

(I need to update this to use go.mod instead as well)

@markmandel markmandel modified the milestones: 0.9.0, 0.10.0 Mar 26, 2019
@markmandel markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Mar 27, 2019
Copy link
Collaborator

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - What kind of operations do we want to support ? GET/POST for sure but have you anything else in mind ?

@cyriltovena
Copy link
Collaborator

Another thing but I'm ok with it, I don't think https is a good package name I understand why but I would prefer that you named it http and import it as https.

The other API extension libraries were too opinionated,
or not flexible enough to provide exactly what we need
-- register a single verb in the API, not have storage, give full control over
the http lifecycle.

This provides just enough functionality for what we need, while also providing
extension points in case we need to expand the API surface area in the future -
such as a more explict OpenAPI spec.

This is the foundational work that is required for converting
GameServerAllocations into an API Extension - next step will
be to use this library to implement that functionality.
@markmandel
Copy link
Member Author

LGTM - What kind of operations do we want to support ? GET/POST for sure but have you anything else in mind ?

So, since we control the entire HTTP response - all HTTP verbs will be passed down to the Handler - then it's up to the handler to decide what to do with them. For example, we do this here: https://github.com/markmandel/agones/blob/feature/gsa-api-server/pkg/gameserverallocations/controller.go#L153-L157

So I guess we already support all of them? 😄

Another thing but I'm ok with it, I don't think https is a good package name I understand why but I would prefer that you named it http and import it as https.

As you may have guessed, this server has to be https, so it felt like https was a good package name. Maybe it should just be something more like web or httpsserver - but that didn't feel very go like? http feels misleading, since it has to be https - maybe http/TLSServer ? I'm not sure 🤷‍♂️ Happy to do a refactor PR if we can come up with a better package name 👍

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 16616f39-92ec-492f-aea7-2df53260e5e3

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/659/head:pr_659 && git checkout pr_659
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.9.0-1063c63

@cyriltovena
Copy link
Collaborator

let's keep it like this, may be later we will have a better view.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 5b8c4217-0900-440b-b229-a8f0da6141ae

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:

  • git fetch https://github.com/GoogleCloudPlatform/agones.git pull/659/head:pr_659 && git checkout pr_659
  • helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=0.10.0-feabed4

@markmandel markmandel merged commit 46578ba into googleforgames:master Apr 3, 2019
@markmandel markmandel deleted the feature/apiserver branch April 3, 2019 15:56
@markmandel markmandel removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New features for Agones
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants