Add conformance tests for .metadata.generateName. #3292
Conversation
knative-prow-robot
added
the
size/L
There was a problem hiding this comment.
@brandon-mabey: 0 warnings.
In response to this:
Fixes #3183
Proposed Changes
- Add testing to ensure that .metadata.generateName can be used instead of .metadata.name to specify names for services, routes, and configurations.
Release Note
NONE
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.
knative-prow-robot
added
the
needs-ok-to-test
|
|
||
| // Ensure that the name given to the configuration is generated from the generate name field. | ||
| t.Log("When the configuration is created, the name is generated using the provided generateName") | ||
| if names.Config == generateName { |
There was a problem hiding this comment.
These statements could probably be refactored out to a common function for Service, Config, and Route to reduce duplication (i.e. validateName(prefix, name string) error . A regex also might be both simpler and capture more requirements as to what we expect from generate name such as:
- What characters should follow the prefix?
- How long should the generated name be?
There was a problem hiding this comment.
Moved to validateName, and changed it to a regex. It now validates that the prefix is followed by 5 lower-case characters or digits, which is what the current behaviour is.
It might be a bit overly strict since it now depends on the api-server implementation not changing. I believe Kubernetes only guarantees a unique suffix (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#idempotency). Is this updated regex okay?
There was a problem hiding this comment.
I would loosen it up a bit to be prefix followed by one or more valid name characters ( https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names )
There was a problem hiding this comment.
Okay, loosed up the regex and changed it. Thanks.
| } | ||
|
|
||
| // generateNamePrefix returns the object name to be used for testing, shorted to | ||
| // 44 characters to avoid #3236, as generateNames longer than 44 characters may cause |
There was a problem hiding this comment.
The issue referenced seems to indicate 49 characters. I assume the text here then is expecting 5 additional from the generateName randomness?
There was a problem hiding this comment.
Yes, that is exactly it. 49 characters will work with a service with .metadata.name set but not .metadata.generateName set.
|
/ok-to-test |
knative-prow-robot
removed
the
needs-ok-to-test
|
@brandon-mabey Can you rebase this? |
brandon-mabey
force-pushed
the
generate-name-conformance
branch
from
691ab9e
to
ed7d4a0
Compare
knative-prow-robot
added
the
area/test-and-release
|
@mattmoor Done. Rebased merge conflict. |
| r := regexp.MustCompile("^" + regexp.QuoteMeta(generateName) + "[a-zA-Z0-9\\-.]+$") | ||
|
|
||
| if !r.MatchString(name) { | ||
| return fmt.Errorf("expected generated name to match \"%v\", got %s", r, name) |
There was a problem hiding this comment.
- got before want e.g
"generated name = %q want to match %q". - use: https://golang.org/pkg/regexp/#Regexp.String and %q as format qualifier.
There was a problem hiding this comment.
Fixed, also edited another instance of "%s" below. Thanks.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brandon-mabey, mattmoor, vagababov 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 |
knative-prow-robot
added
the
approved
Fixes #3183
Proposed Changes
Release Note