CLOUDP-93590: Service name configurable #616
Conversation
FlorentinaSimlinger
changed the title
FlorentinaSimlinger
changed the title
test/e2e/statefulset_arbitrary_config/statefulset_arbitrary_config_test.go
Outdated
Show resolved
Hide resolved
FlorentinaSimlinger
requested review from
bznein,
chatton,
priyolahiri,
rodrigovalin and
ciprian-tibulca
as code owners
api/v1/mongodbcommunity_types.go
Outdated
| @@ -523,8 +523,12 @@ func (m MongoDBCommunity) Hosts() []string { | |||
| } | |||
|
|
|||
| // ServiceName returns the name of the Service that should be created for | |||
| // this resource | |||
| // this resource which is either the default name resourcename-svc or the user defined name | |||
There was a problem hiding this comment.
[nit] I don't think implementation details necessarily need to be included in the docstring. The new text added is re-stating the body of the function. What do you think?
| @@ -289,6 +289,19 @@ func BasicFunctionality(mdb *mdbv1.MongoDBCommunity) func(*testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // ServiceWithNameExists checks whether a service with the name serviceName exists | |||
| func ServiceWithNameExists(mdb *mdbv1.MongoDBCommunity, serviceName string) func(t *testing.T) { | |||
There was a problem hiding this comment.
[nit] we are introducing a dependency on *mdbv1.MongoDBCommunity when in realsity all we actually care about is the namespace. This signature could be (namespace, serviceName string) or even (nsName types.NamespacedName)
There was a problem hiding this comment.
This should happen more often in functions, I think mongodbtests.go has quite a few such examples -- e.g StatefulSetHasOwnerReference ...
| tester, err := mongotester.FromResource(t, mdb) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| t.Run("Create MongoDB Resource", mongodbtests.CreateMongoDBResource(&mdb, ctx)) | ||
| t.Run("Basic tests", mongodbtests.BasicFunctionality(&mdb)) | ||
| t.Run("Test setting Service Name", mongodbtests.ServiceWithNameExists(&mdb, serviceName)) |
There was a problem hiding this comment.
we are now only testing the service creation when it is specified in the StatefulSetSpec override, we could add this to the BasicTests tests which are run in every test to make sure the service name is as expected when not overriding through sts override. WDYT?
There was a problem hiding this comment.
do you mean adding a call to ServiceWithNameExists to BasicFunctionality and call it with m.Name + "-svc"?
There was a problem hiding this comment.
The overall implementation LGTM, I have only one main question here: what happens if a user here creates two resources with the same name service name?
I guess this is the user's responsibility so no need to implement weird logic to cover this case, but is there maybe a clean way to identify this situation?
Other two minor things:
- We should add it to the release notes doc
- We could maybe provide an example in the
samplesdirectory
How would that be possible? you mean create the service manually? |
I mean creating two resources and use the same value for both in |
In this case, the reconcile should just fail and report it to status -- which is what will happen currently. |
SGTM! |
| customServiceName := "database" | ||
| mdb.Spec.StatefulSetConfiguration.SpecWrapper.Spec.ServiceName = customServiceName |
There was a problem hiding this comment.
I changed the parameter to customServiceName to make it clearer to the reader that this is a name that is set by the user. Happy to revert if original parameter name is preferred.
FlorentinaSimlinger
force-pushed
the
CLOUDP-93590_service_name_configurable
branch
from
15b2a6f to
f554508
Compare
All Submissions:
closes #XXXXin your comment to auto-close the issue that your PR fixes (if such).Tested by adding
statefulSet:
spec:
serviceName: database
to the custom resource yaml file and running make run to observe that the serviceName field now contains database and by setting the service name in statefulset_arbitrary_config_test.go and checking the name is correctly set in mongodbtests.go