Closed jhadvig closed 3 months ago
@jhadvig: This pull request references CONSOLE-4163 which is a valid jira issue.
Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.
Hello @jhadvig! Some important instructions when contributing to openshift/api: API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.
Thanks @spadgett for comment.
I think it might be better to use the enum as suggested in the PR (hide/show
), since that better reflects how the widgets are presented in the UI.
eg. if we would use the enum types from the PerspectiveState
or CatalogTypesState
, it should be misleading for the LightspeedButtom case, in which the Disabled
state could mean that the button is visible, but is disabled.
I can see that one of the WidgetStates
could be Disabled
or even Enabled
, but for the purpose of the Lightspeed I think Show/Hide
does more reflect the expected states.
I'm wondering if we want something more akin to:
widgets:
enabled:
- LightSpeedButton
disabled:
- MyOtherWidget
You could use CEL to ensure the two lists are disjoint, you'd need to document each potential value and what the default state of the widget is
You could use a string alias with an enum tag to ensure the list only contains pre-defined values
@JoelSpeed thank you for comment.
You could use CEL to ensure the two lists are disjoint, you'd need to document each potential value and what the default state of the widget is.
Thats the problem that its hard to estimate what potential values we will have. I can imagine that the enum could hold any HTML element attribute potentially. Also the proposed API wont need to worry about the disjoint list. The only thing Im not really sure is if we can change the default value over the time, if thats would be a breaking change? But even that could be handled programmatically on operator level, if so.
With the API I proposed, I wouldn't expect you to actually default the value inside the API, but handle the defaults through documentation and the operator itself.
I'm thinking something along the lines of specifying which are enabled unless explicitly disabled, WDYT?
// +kubeubilder:validation:Enum:=LightSpeedButton;GettingStartedModal;MyOtherThing
type WidgetName string
type Widgets struct {
// enabled determines which widgets should or should not be enabled.
// When a widget is enabled, it will be shown within the console UI.
// Available widgets are LightSpeedButton, GettingStartedModal and MyOtherThing.
// When omitted, widgets will assume a default state which is subject to change over time.
// The following widgets are enabled unless explicitly disabled: LightSpeedButton, MyOtherThing.
Enabled []WidgetName `json:"enabled"`
...
}
If a widget is not enabled, and it's not disabled, what else could it be? Could a widget possibly be conditionally enabled? Is there a need for structured enablement of widgets? If we think there will ever need to be structure to this, then the enabled/disabled lists won't work and yes, something more structured would be required. Something like the perspectives probably?
@JoelSpeed I've update the PR based on our conversation, to make the Widgets more extensible in the future.
The way how we will default the LightspeedButton
is that we will update the console-operator's manifest by adding the default
widgets:
- name: LightspeedButton
visibility:
state: Show
PTAL
@JoelSpeed comments addressed. PTAL
Shape looks good, one more question on documentation, but otherwise happy to proceed. Can we get the console operator PR updated with this and go through the QE pre-merge process?
@JoelSpeed here is the operator PR
@spadgett @JoelSpeed I've added the generated pieces of the work. Here is the console-operator PR with vendored changes, code changes and unit tests. PTAL
@JoelSpeed comments addressed. PTAL :)
@jhadvig: This pull request references CONSOLE-4161 which is a valid jira issue.
Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set.
/jira refresh
@jhadvig: This pull request references CONSOLE-4161 which is a valid jira issue.
Since we are so close to the feature freeze date, I'd like to request that QE pre-merge verify this API in combination with the console operator PR that implements the feature, before we merge the API. So that we can iron out any issues and not risk an API merged without the implementation likely to merge
ccing @yapei
/label qe-approved
@jhadvig: This pull request references CONSOLE-4161 which is a valid jira issue.
@jhadvig: The following tests failed, say /retest
to rerun all failed tests or /retest-required
to rerun all mandatory failed tests:
Test name | Commit | Details | Required | Rerun command |
---|---|---|---|---|
ci/prow/e2e-upgrade-minor | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-upgrade-minor |
ci/prow/e2e-aws-serial-techpreview | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-aws-serial-techpreview |
ci/prow/e2e-aws-ovn | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-aws-ovn |
ci/prow/e2e-aws-ovn-techpreview | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-aws-ovn-techpreview |
ci/prow/e2e-aws-serial | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-aws-serial |
ci/prow/e2e-upgrade | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-upgrade |
ci/prow/e2e-aws-ovn-hypershift | 8de1724b55578a2f84d4c53ae75acfa360e2531c | link | true | /test e2e-aws-ovn-hypershift |
Full PR test history. Your PR dashboard.
/lgtm
This is based on QE approval, please make sure the operator PR also merges, if it does not merge in time for 4.17 cut off, I will have to revert the API changes as well and re-introduce it in 4.18
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: jhadvig, JoelSpeed
The full list of commands accepted by this bot can be found here.
The pull request process is described here
[ART PR BUILD NOTIFIER]
Distgit: ose-cluster-config-api This PR has been included in build ose-cluster-config-api-container-v4.17.0-202407301715.p0.g0d46442.assembly.stream.el9. All builds following this will include this PR.
Adding new
widgets
field into theConsoleCustomization
struct. The purpose of this field is to have set of predefined UI widgets, in this case the Lightspeed button, and their state. The Lightspeed button should serve as a cluster wide setting for showing/hidding the button to all cluster users. Thewidgets
field can also be easily extended with additional widgets (buttons, icons, ...) Pros:lightspeedButton
field will be part of the config by defaultwidgets
can be easily extended with new UI widgets/elementsCons:
Other possible solution would be to have an array of widgets:
In this case we wont need to worry about the deprecation of any predefined widget, simply if we wont need it anymore, or there will be a particular change ,we can just leave it out of the
widgets
array. On the other hand the Lightspeed button wont be part of the default config, instead it will need to be part of all the console-operator's config when creating cluster./assign @JoelSpeed
cc'ing @spadgett @rhamilto