Require EDPMServiceType #1176
Require EDPMServiceType #1176
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bshephar 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 |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/278066ea4b734db9a37b3e10a5a15889 ❌ openstack-k8s-operators-content-provider FAILURE in 5m 10s |
bshephar
force-pushed
the
EdpmServiceType-webhook
branch
from
44af400 to
0cd0898
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/d06be16b184c4f58ade975c692a654f9 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 46m 22s |
bshephar
force-pushed
the
EdpmServiceType-webhook
branch
from
0cd0898 to
a88cadf
Compare
| @@ -112,7 +112,7 @@ spec: | |||
| deployOnAllNodeSets: true | |||
| ---- | |||
|
|
|||
| . Optional: Specify the `edpmServiceType` field for the service. Different custom services may use the same ansible content to manage the same EDPM service (such as `ovn` or `nova`). The `DataSources`, TLS certificates, and CA certificates need to be mounted at the same locations so they can be found by the ansible content even when using a custom service. `edpmServiceType` is used to create this association. The value is the name of the default service that uses the same ansible content as the custom service. If there are multiple services with the same `edpmServiceType` listed in a nodeset or deployment spec, latter ones would be ignored. | |||
| . Required: Specify the `edpmServiceType` field for the service. Different custom services may use the same ansible content to manage the same EDPM service (such as `ovn` or `nova`). The `DataSources`, TLS certificates, and CA certificates need to be mounted at the same locations so they can be found by the ansible content even when using a custom service. `edpmServiceType` is used to create this association. The value is the name of the default service that uses the same ansible content as the custom service. If there are multiple services with the same `edpmServiceType` listed in a nodeset or deployment spec, latter ones would be ignored. | |||
There was a problem hiding this comment.
It's not actually required that edpmServiceType matches that of an existing service. It's really just if you are re-using existing ansible content from epdm-ansible, and need to re-use the same paths for cert mounts. So, I think that needs to be clarified. The field can still be required though, so that we can force the user to make that distinction when they create the service.
There was a problem hiding this comment.
I'm still kind of on the fence as to whether we should do this or not. Maybe it can just be improved in documentation. But I'll note my thoughts on the topic below since we have a thread here related to it.
The only thing that concerns me with the current implementation is that it doesn't work if users try to create a new Nova service for example, but fail to specify edpmServiceType: nova. I think it probably improves the user experience by requiring the field. But then at the same time, it also puts more of a mental burden on them to now go and understand what this edpmServiceType means.
I think it's probably easier for the user if we block the CR creation by requiring this field, then they have to go and read about the parameters. Whereas the alternative is having to debug why the deployment failed, which is technically more challenging / a worse user experience.
I agree with this comment, I'm just dropping my thoughts on the thread. I'll do some rewriting of this section to include that clarification.
bshephar
force-pushed
the
EdpmServiceType-webhook
branch
2 times, most recently
from
2559460 to
516d2c1
Compare
bshephar
force-pushed
the
EdpmServiceType-webhook
branch
3 times, most recently
from
5a47593 to
9559d05
Compare
This change switches EDPMServiceType from optional to required. Since the EDPMServiceType of custom services needs to match that of an existing service, we also remove the defaulting mechanism in the webhook. Signed-off-by: Brendan Shephard <[email protected]>
bshephar
force-pushed
the
EdpmServiceType-webhook
branch
from
9559d05 to
76505cb
Compare
|
/test functional |
|
Functional tests pass locally, rechecking it: |
|
So, the remaining problem we have here is that in making this field mandatory, we risk the update process. Since CR's defined without the field will be un-upgradable until the field is defined. I don't think this should be an issue, since we were adding the field with a validation webhook, so all existing CR's in the cluster should have the field. But we should consider it as a potential upgrade issue and test for it accordingly. |
|
/test openstack-operator-build-deploy-kuttl |
|
@bshephar: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
All Kuttl tests seem to be having problems with Quay: |
This change switches EDPMServiceType from optional to required. Since the EDPMServiceType of custom services needs to match that of an existing service, we also remove the defaulting mechanism in the webhook.
jira: https://issues.redhat.com/browse/OSPRH-11333