-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
pkg/test/context.go: create an id that is a valid namespace name #2335
pkg/test/context.go: create an id that is a valid namespace name #2335
Conversation
pkg/test/resource_creator.go
Outdated
@@ -34,7 +35,7 @@ func (ctx *TestCtx) GetNamespace() (string, error) { | |||
return ctx.namespace, nil | |||
} | |||
// create namespace | |||
ctx.namespace = ctx.GetID() | |||
ctx.namespace = fmt.Sprintf("osdk-e2e-%s", uuid.New()) |
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'd like to investigate adding unit tests around GetNamespace this weekend. Need to figure how to handle testing code using UUID. Attach a UUID on TestCtx? Maybe change GetID() to return this UUID on TestCtx instead of creating the UUID here.
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.
Taking a second look at this: I think assigning ctx.id in newTestCtx to the above string would make this all easier. So instead of changing resource_creator modify pkg/test/context.go's newTestCtx.
38d189f
to
dd1cdd8
Compare
dd1cdd8
to
df69a19
Compare
|
||
id := prefix + "-" + strconv.FormatInt(time.Now().Unix(), 10) | ||
// TestCtx is used among others for namespace names where '/' is forbidden and must be 63 characters or less | ||
id := "osdk-e2e-" + uuid.New() |
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 like this approach much better than my previous code change. This weekend I'd still like to add unit tests around NewTestCtx, but please let me know thoughts on this approach.
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.
It is ok for me. @joelanford have you any objection?
ed51bc6
to
6938a11
Compare
} | ||
|
||
id := prefix + "-" + strconv.FormatInt(time.Now().Unix(), 10) | ||
// TestCtx is used among others for namespace names where '/' is forbidden and must be 63 characters or less |
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.
@joelanford wdyt? Are you ok with?
For me, it shows fine.
CHANGELOG.md
Outdated
@@ -25,6 +25,7 @@ | |||
|
|||
### Bug Fixes | |||
- Fix `operator-sdk build`'s `--image-build-args` to support spaces within quotes like `--label some.name="First Last"`. ([#2312](https://github.com/operator-framework/operator-sdk/pull/2312)) | |||
- Fix `pkg/test/NewTestCtx()` to always create an id that is a valid namespace name. This prevents `TestCtx.GetNamespace()` from attempting to sometimes erroneously creating a namespace with a name exceeding 63 characters. ([#2335](https://github.com/operator-framework/operator-sdk/pull/2335)) |
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.
WDYT?
- Fix `pkg/test/NewTestCtx()` to always create an id that is a valid namespace name. This prevents `TestCtx.GetNamespace()` from attempting to sometimes erroneously creating a namespace with a name exceeding 63 characters. ([#2335](https://github.com/operator-framework/operator-sdk/pull/2335)) | |
- Fix issue to use from test-framework which is faced when the created namespace is exceeding 63 characters. The `TestCtx.GetNamespace()` was changed to always create a unique ID instead of using the operator name in order to avoid this scenario. ([#2335](https://github.com/operator-framework/operator-sdk/pull/2335)) |
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 for the suggestion! I made a couple additional modifications to the proposed change. Please let me know what you think.
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.
Hi @dustinspecker,
Really tks for your contribution. It shows OK for me. Just a nit in the CHANGELOG.
Also, please could you rebase it with the master branch?
6938a11
to
40c907c
Compare
@@ -45,8 +45,8 @@ func TestMemcached(t *testing.T) { | |||
} | |||
// run subtests | |||
t.Run("memcached-group", func(t *testing.T) { | |||
t.Run("Cluster", MemcachedCluster) | |||
t.Run("Cluster2", MemcachedCluster) | |||
t.Run("AVeryLongClusterNameThatIsSoLong", MemcachedCluster) |
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.
Why are these names changed?
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 think he tried to demonstrate that the issue was solved.
However, I agree with @estroz. I would stick with the previous names either.
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.
Yep, that's why I did it. Before this change that test would fail. I'll change it back.
Before this change it was possible for `TestCtx.GetNamespace()` to create a namespace with too long of a name, thus getting an error from Kubernetes saying the namespace name exceeded 63 characters. Now set the id to "osdk-e2e-UUID", so that `TestCtx.GetNamespace()` is guaranteed to have a namespace name be 63 characters or less, which is a valid namespace name. Fixes operator-framework#2101
40c907c
to
176030b
Compare
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.
It shows ok for me
/lgtm
/approve
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.
/lgtm
Before this change it was possible for
TestCtx.GetNamespace()
to create anamespace with too long of a name, thus getting an error from Kubernetes
saying the namespace name exceeded 63 characters.
Now set the id to "osdk-e2e-UUID", so that
TestCtx.GetNamespace()
isguaranteed to have a namespace name be 63 characters or less, which is a
valid namespace name.
Fixes #2101