Allow users to specify their container name.#4289
Allow users to specify their container name.#4289knative-prow-robot merged 4 commits intoknative:masterfrom
Conversation
knative-prow-robot
added
size/XL
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
vagababov
left a comment
vagababov
left a comment
There was a problem hiding this comment.
I think change to the spec is missing, where we specify that at least queue-proxy is a reserved name.
Otherwise mostly nits.
|
/lint |
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@vagababov: 2 warnings.
Details
In response to this:
/lint
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
I had to rework this a bit due to the |
|
Alright, 🤞 that it all works now. |
knative-prow-robot
added
the
area/test-and-release
|
/retest |
With this change, users can specify the `name:` of the container they specify
in their revision spec. This is to ease migration from K8s abstractions of
a similar shape where this field is required, and to lessen the eyesore of the
yaml returned by the API server, which doesn't `omitempty`.
The default value for container name is configurable via `config-defaults`, but
defaults to `user-container` for consistency with what we have today. The
container name default in `config-defaults` is a Go template, which has access
to the ObjectMeta of the enclosing resource's ObjectMeta (e.g. Service,
Configuration), so if an operator wanted to make the container name match the
enclosing Service, they may set this to `{{.Name}}`.
Fixes: knative#4257
Add `name:` to spec.md
mattmoor
force-pushed
the
allow-name
branch
2 times, most recently
from
335d1a1 to
13c9a37
Compare
|
Alright, upgrade should be fixed for realz. The problem was a bad merge with Jon's change, which merged "cleanly" with mine. 🙄 |
|
still fails :) |
|
Ugh, he put it in a separate file 🙄 (shakes fist at Jon lovingly for adding all these tests) |
Also tweak the readiness logic to look for URL instead of Domain.
|
Ok, should be fixed now. (famous last words) |
|
The following is the coverage report on pkg/.
|
6 participants
With this change, users can specify the
name:of the container they specifyin their revision spec. This is to ease migration from K8s abstractions of
a similar shape where this field is required, and to lessen the eyesore of the
yaml returned by the API server, which doesn't
omitempty.The default value for container name is configurable via
config-defaults, butdefaults to
user-containerfor consistency with what we have today. Thecontainer name default in
config-defaultsis a Go template, which has accessto the ObjectMeta of the enclosing resource's ObjectMeta (e.g. Service,
Configuration), so if an operator wanted to make the container name match the
enclosing Service, they may set this to
{{.Name}}.This also pulls the user annotation stuff into v1beta1, which was apparently still TODO.
Fixes: #4257