add PublicViewerConfig in ToolchainConfig #410
Conversation
Signed-off-by: Francesco Ilario <filario@redhat.com>
Signed-off-by: Francesco Ilario <filario@redhat.com>
filariow
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
| // +k8s:openapi-gen=true | ||
| type GlobalConfig struct { | ||
| // +optional | ||
| PublicViewer PublicViewerConfig `json:"publicViewer,omitempty"` |
There was a problem hiding this comment.
It seems like a good idea to start introducing a Global/Common config but I'm wondering if we could add the PublicViewer struct to both HostConfig and Members, instead of creating a Global config? Is there any benefit for having this?
There was a problem hiding this comment.
This configuration is used by host operator and registration service. Neither HostConfig nor MembersConfig seemed appropriate to me.
Adding the configuration in both of them does not sound a good idea to me. It would become possible to have two different values for the same configuration and cause misbehavior.
As @MatousJobanek suggested here we could add this config under HostConfig. My only concern about going this path is related to Registration Service's future improvements. Indeed, as of now, the RegistrationService is thought to be installed on the Host cluster only, but it seems to me that it may be easily decoupled so that it can be executed on member clusters too. So, adding this configuration under HostConfig may cause breaking changes or duplication if we decide to decouple the RegistrationService from the Host Cluster. WDYT?
There was a problem hiding this comment.
the RegistrationService is thought to be installed on the Host cluster only, but it seems to me that it may be easily decoupled so that it can be executed on member clusters too
Why would you want to install the reg service on a member cluster? I don't think we ever considered that. But maybe I missed something?
What would make sense is to decouple the proxy and reg-service so the proxy can be an optional component but it's still all about the host cluster components. Isn't it?
There was a problem hiding this comment.
Sorry, probably I didn't express myself correctly. I'm not saying that's something we are planning or we should plan. I'm saying that the fact that it's technically doable, makes me feel like we can find better place where to put this configuration 😄
| // +kubebuilder:default:=false | ||
| Enabled bool `json:"enabled"` |
There was a problem hiding this comment.
what is the purpose of the boolean flag? Why the username is not sufficient? If the username is empty, then it's disabled, if it's not empty then it's enabled...
There was a problem hiding this comment.
the username would be sufficient, but not explicit. Hence, the purpose of the boolean enabled field is to make the feature flag explicit
There was a problem hiding this comment.
An alternative we could pursue down the line would be to have GlobalConfig have a pointer to an instance of this struct, with the nil case indicating that this particular feature isn't enabled. However, this has the downside of being less explicit about which features are enabled, and it breaks the encapsulation of PublicViewerConfig (i.e. if more stuff needs to be added to this struct orthogonal to the user, then we're going to need to reconsider our approach).
Signed-off-by: Francesco Ilario <filario@redhat.com>
There was a problem hiding this comment.
@filariow Can you include a link to a JIRA that provides background for these changes? I'm pretty lost as to what these changes are about. Not sure what a public viewer is and not sure why we need a Global config struct when we have HostConfig and MemberConfigs.
| @@ -26,6 +26,9 @@ const ( | |||
|
|
|||
| // SpaceBindingRequestProvisionedReason represents the reason for a successfully provisioned space binding request. | |||
| SpaceBindingRequestProvisionedReason = provisionedReason | |||
|
|
|||
| // SpaceBindingRequestInvalidReason represents the reason for an invalid space binding request. | |||
| SpaceBindingRequestInvalidReason = invalidReason | |||
There was a problem hiding this comment.
Shouldn't the creation of the resource be blocked if it's invalid? Why allow creation of an invalid resource?
There was a problem hiding this comment.
great point, this check could be implemented in the ValidatingWebhook on member cluster. WDYT?
|
@rajivnathan sorry, I forgot to add the link. Added it in the description. |
Signed-off-by: Francesco Ilario <filario@redhat.com>
|
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$` | ||
| Username string `json:"username,omitempty"` |
There was a problem hiding this comment.
What is this username used for? Can you please add a comment here describing this field.
Thanks for the JIRA link. However the JIRA and the attached feature refinement doc describes the high level desired UX. What would be really useful is a detailed technical design doc. It would help a lot with the review of the PR.
|
Description
JIRA: https://issues.redhat.com/browse/ASC-492
Checks
Did you run
make generatetarget? yesDid
make generatechange anything in other projects (host-operator, member-operator)? yes