-
Notifications
You must be signed in to change notification settings - Fork 817
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
Add Minikube instructions #321
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Signed the thing |
CLAs look good, thanks! |
Build Succeeded 👏 Build Id: 640776c7-5fa9-4151-ad05-5dbed299124a The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
Build Succeeded 👏 Build Id: 217a8149-057f-42ea-a061-79a60b423d0e The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
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.
Overall, this looks great 🎉
Had some thoughts/questions below, lemme know what you think!
Thanks!!!
docs/edit_first_game_server.md
Outdated
|
||
To install on GKE, follow the install instructions (if you haven't already) at [Install and configure Agones on Kubernetes](../install/README.md). At least, you should have "Setting up a Google Kubernetes Engine (GKE) cluster", "Enabling creation of RBAC resources" and "Installing Agones" done. | ||
|
||
To install locally on Minikube, read [Developing, Testing, and Building Agones](../build/README.md) making sure to follow the instructions in the [Running A Test Minikube cluster](../build/README.md#running-a-test-minikube-cluster) section. |
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.
Should we link to installing on Minikube instead?
https://github.com/GoogleCloudPlatform/agones/blob/master/install/README.md#setting-up-a-minikube-cluster
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.
Pros seem to be that it is less complex and requires less reading time.
Cons are that the reader is left to discover build tools on their own, and they might need information about having the project on their gopath.
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.
So I would argue that if they are developing with Agones - they shouldn't need the build tools - they should install and use Agones as per normal.
I'm very hesitant to have the streams of using Agones and Developing Agones mix - they have very different targets, and it could get super messy in the long term.
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.
Per your suggestion, this now links to https://github.com/GoogleCloudPlatform/agones/blob/master/install/README.md#setting-up-a-minikube-cluster
docs/edit_first_game_server.md
Outdated
3. Follow the install instructions (if you haven't already) at [Install and configure Agones on Kubernetes](../install/README.md). At least, you should have "Setting up a Google Kubernetes Engine (GKE) cluster", "Enabling creation of RBAC resources" and "Installing Agones" done. | ||
3. Install Agones on GKE or Minikube. | ||
|
||
To install on GKE, follow the install instructions (if you haven't already) at [Install and configure Agones on Kubernetes](../install/README.md). At least, you should have "Setting up a Google Kubernetes Engine (GKE) cluster", "Enabling creation of RBAC resources" and "Installing Agones" 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.
We can link to the GKE section directly:
https://github.com/GoogleCloudPlatform/agones/blob/master/install/README.md#setting-up-a-google-kubernetes-engine-gke-cluster
WDYT?
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.
Yes, that would be more consistent.
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.
This change has been made.
docs/edit_first_game_server.md
Outdated
|
||
### Create a new docker image | ||
```bash | ||
docker build -t gcr.io/[PROJECT_ID]/agones-udp-server:0.2 . | ||
``` | ||
Note that: you can change the image name "agones-udp-server" to something else. | ||
|
||
### Push the image to GCP Registry | ||
### If using GKE, push the image to GCP Registry | ||
```bash | ||
docker push gcr.io/[PROJECT_ID]/agones-udp-server:0.2 |
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.
Wondering if we should link to:
https://cloud.google.com/container-registry/docs/advanced-authentication
To point people in the right direction to authenticate against the GCR?
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.
Do you mean add content along the lines of "More information is available at "?
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.
Committed some text adding this link.
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.
Perfect 👍
docs/edit_first_game_server.md
Outdated
```bash | ||
docker push gcr.io/[PROJECT_ID]/agones-udp-server:0.2 | ||
``` | ||
|
||
### Modify gameserver.yaml | ||
### If using Minikube, change directory to agones/build and push the image into Minikube | ||
```bash |
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.
Rather than point people to development tools, we can just show them the command line trick:
(just concerned with Agones users relying on development tooling - in case it changes).
docker save [PROJECT_ID]/agones-udp-server:0.2 | (eval $(minikube docker-env) && docker load)
WDYT?
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.
Yes that makes sense. It seems less complex. I tried both ways and eval $(minikube docker-env) etc... worked fine provided the make tools weren't used.
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.
I'm thinking we shouldn't be pointing people towards the development tooling - it's very cross purpose, and developers shouldn't be coupled to our own development tooling.
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.
The doc now uses docker save [PROJECT_ID]/agones-udp-server:0.2 | (eval $(minikube docker-env) && docker load) instead of make.
docs/edit_first_game_server.md
Outdated
containers: | ||
- name: agones-simple-udp | ||
image: gcr.io/[PROJECT_ID]/agones-udp-server:0.2 | ||
imagePullPolicy: Never |
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.
Do we need imagePullPolicy: Never
? it is the default.
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.
I found I needed it or I would get an error as it failed to pull the image. I'll try without it when not using make.
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.
Hmn. That's weird. Actually, I lie - Never
is not the default IfNotPresent
- but if you have transferred the image to Minikube, it shouldn't pull it then.
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.
imagePullPolicy: Never has been removed. Now both GKE and Minikube share the same paragraph.
Build Succeeded 👏 Build Id: e4c15136-db1c-4774-8657-c2c3d830ea60 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
Build Succeeded 👏 Build Id: 30a24203-7cc8-4457-be35-538136e7ad06 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
Build Succeeded 👏 Build Id: 287cb09e-30fd-48f6-b317-2e5751b354ae The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
LGTM! Can we rebase down to a single commit, and I'll give it the stamp of approval and merge! THANKS! |
8291a50
to
29f53bb
Compare
I have some text to cleanup from merging. I left some git stuff in the document that needs to be removed. |
29f53bb
to
4cf4b2c
Compare
Build Succeeded 👏 Build Id: f6832e79-668e-451d-8354-39a21d106d2f The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
It's cleaned up and rebased to a single commit. Next time I will be more careful squashing :) |
Add Minikube instructions to edit_first_game_server.md