-
Notifications
You must be signed in to change notification settings - Fork 819
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 pod annotations #1849
Add pod annotations #1849
Conversation
Build Succeeded 👏 Build Id: eeca5afe-1483-4505-9b2c-4f7d371b5f69 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:
|
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.
Looks great! Just one small tweak in the docs.
Not sure if you want to - but you could also do the Ping service while you are in there?
https://github.com/googleforgames/agones/blob/master/install/helm/agones/templates/ping.yaml
@@ -186,6 +186,7 @@ The following tables lists the configurable parameters of the Agones chart and t | |||
| `agones.controller.nodeSelector` | Controller [node labels][nodeSelector] for pod assignment | `{}` | | |||
| `agones.controller.tolerations` | Controller [toleration][toleration] labels for pod assignment | `[]` | | |||
| `agones.controller.affinity` | Controller [affinity][affinity] settings for pod assignment | `{}` | | |||
| `agones.controller.annotations` | Controller [annotations][annotations] settings for pod assignment | `{}` | |
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.
Please move this down to where we have the section "New Configuration Features:" - so they only get displayed on the next release 👍
Context: https://agones.dev/site/docs/contribute/#within-a-page
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.
Thank you. I fixed it with ab93fd6.
@@ -226,6 +227,7 @@ The following tables lists the configurable parameters of the Agones chart and t | |||
| `agones.allocator.disableTLS` | Turns off TLS security for incoming connections to the allocator. | `false` | | |||
| `agones.allocator.tolerations` | Allocator [toleration][toleration] labels for pod assignment | `[]` | | |||
| `agones.allocator.affinity` | Allocator [affinity][affinity] settings for pod assignment | `{}` | | |||
| `agones.allocator.annotations` | Allocator [annotations][annotations] settings for pod assignment | `{}` | |
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.
Please also move this down to where we have the section "New Configuration Features:" - so they only get displayed on the next release 👍
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.
Thank you. I fixed it with ab93fd6.
Do you mean Only ping Deployment doesn't have annotations, which seems strange, so I'll add them together. |
Build Failed 😱 Build Id: c614896e-770d-4ffa-91de-3b3082170344 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Not quite seeing it, but there something going wrong with the annotation link:
If you want to test it yourself |
Oh, moving annotations to "New Configuration Features" seems to have affected it. I'm trying the |
Build Failed 😱 Build Id: eeafd610-2941-4664-9026-fa71665c0d68 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
It's still no good... I've written a patch to run the diff --git a/build/Makefile b/build/Makefile
index 99cb5843..aa7bb85d 100644
--- a/build/Makefile
+++ b/build/Makefile
@@ -235,6 +235,8 @@ push-release-chart: $(ensure-build-image)
# Run all tests
test: $(ensure-build-image) test-go test-sdks test-install-yaml site-test
+site-test: $(ensure-build-image) site-test
+
# Run go tests
test-go: $(ensure-build-image)
$(GO_TEST) $(agones_package)/pkg/... \ |
| `agones.controller.annotations` | Controller [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.allocator.annotations` | Allocator [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.ping.annotations` | Ping [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.ping.http.annotations` | [Annotations](annotations) for ping http service | `{}` | |
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.
[Annotations](annotations)
😭
my typo...
124c261
to
b707bb0
Compare
Build Succeeded 👏 Build Id: 67da3b06-d3f0-4a47-8160-4d97f2d024e6 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:
|
| `agones.controller.annotations` | Controller [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.allocator.annotations` | Allocator [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.ping.annotations` | Ping [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.ping.http.annotations` | [Annotations][annotations] for ping http service | `{}` | |
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.
Sorry to make you juggle this around, but agones.ping.udp.annotations
and agones.ping.http.annotations
already exist in the current release, but aren't documented - so they can go in the section up above.
There new, remaining values should stay down here.
Random thought - do we think there is any confusion given that agones.ping.udp.annotations
is a annotation on a service, but agones.controller.annotations
is on Pods?
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.
Random thought - do we think there is any confusion given that agones.ping.udp.annotations is a annotation on a service, but agones.controller.annotations is on Pods?
Hmmm, it's somewhat confusing.
Should I revise it to agones.controller.podAnnotations
or agones.ping.udp.serviceAnnotations
or something?
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.
Sorry to make you juggle this around, but agones.ping.udp.annotations and agones.ping.http.annotations already exist in the current release, but aren't documented - so they can go in the section up above.
There new, remaining values should stay down here.
I understand.
I'll fix the name I asked for in my comment above when I decide what to do with it.
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.
@roberthbailey @pooneh-m would love a second opinion?
Worth noting, agones.ping.udp.annotations
and agones.ping.http.annotations
where in previous releases, but weren't documented.
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.
If you look at the structure in https://github.com/googleforgames/agones/blob/master/install/helm/agones/values.yaml I don't actually think it's all that confusing. The annotations for the service / pod are clearly nested under the values for the service / pod respectively.
I think the descriptions in this table should make it clear what type of k8s resource the annotations are being applied to. e.g.
| agones.controller.annotations
| [Annotations][annotations] added to the Agones controller pods |
| agones.ping.http.annotations
| [Annotations][annotations] added to the Agones ping http service |
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.
SGTM! Thanks for the second/third pair of eyes - that works for me 👍
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.
Fixed 👍
@@ -236,7 +236,14 @@ The following tables lists the configurable parameters of the Agones chart and t | |||
|
|||
| Parameter | Description | Default | | |||
| --------------------------------------------------- | ----------------------------------------------------------------------------------------------- | ---------------------- | | |||
| | | | | |||
| `agones.controller.annotations` | Controller [annotations][annotations] settings for pod assignment | `{}` | |
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.
| `agones.controller.annotations` | Controller [annotations][annotations] settings for pod assignment | `{}` | | |
| `agones.controller.annotations` | [Annotations][annotations] added to the Agones controller pods | `{}` | |
Just going by Robbie's suggestion, to make it super clear. 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.
Fixed 👍
| `agones.controller.annotations` | Controller [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.allocator.annotations` | Allocator [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.ping.annotations` | Ping [annotations][annotations] settings for pod assignment | `{}` | | ||
| `agones.ping.http.annotations` | [Annotations][annotations] for ping http service | `{}` | |
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.
| `agones.ping.http.annotations` | [Annotations][annotations] for ping http service | `{}` | | |
| `agones.ping.http.annotations` | [Annotations][annotations] added to the Agones ping http service | `{}` | |
Minor tweaks for just some extra clarity based on Robbie's suggestions. Just to make things extra clear.
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.
Fixed 👍
Build Succeeded 👏 Build Id: 59c57d02-8cf7-4cc6-bb26-ff5496334897 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:
|
Build Succeeded 👏 Build Id: d3f465e3-8302-483e-ad9b-2d96ad932e66 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:
|
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.
Let's get this in!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 8398a7, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: c78afe37-fe54-4d67-8ad6-f0f74df02d23 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:
|
* Changed to allow annotations to be injected (googleforgames#1848) * Docs: Added explanation of annotations (googleforgames#1848) Co-authored-by: Mark Mandel <markmandel@google.com>
What type of PR is this?
/kind feature
What this PR does / Why we need it:
refs: #1848
Which issue(s) this PR fixes:
Closes #1848
Special notes for your reviewer: