-
Notifications
You must be signed in to change notification settings - Fork 613
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
CONSOLE-4269: add custom ConsolePlugin resource details page #14487
Conversation
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
/** | ||
* An optional description that will appear in the title popover. | ||
*/ | ||
description?: string; |
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.
Discussed with @TheRealJon. If description
isn't supplied, the popover is populated with a description from the resource, which likely doesn't make sense, especially in cases like this where the item isn't in the schema.
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
ce82a9b
to
be219d6
Compare
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@vojtechszocs, I removed the changes to |
const labels = enabled ? t('console-app~Enabled') : t('console-app~Disabled'); | ||
|
||
return ( | ||
<> | ||
{consoleOperatorConfig && canPatchConsoleOperatorConfig ? ( | ||
{consoleOperatorConfigLoaded && canPatchConsoleOperatorConfig && !developmentMode ? ( |
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.
Fixes regression introduced in #14403. We only want to display the button when not in dev mode.
@@ -257,15 +279,16 @@ const ConsolePluginsTable: React.FC<ConsolePluginsTableProps> = ({ obj, rows, lo | |||
({ name, version, description, status, enabled, errorMessage, hasCSPViolations }) => ( | |||
<Tr key={name}> | |||
<Td dataLabel={columns[0].id}> | |||
<ResourceLink groupVersionKind={consolePluginGVK} name={name} hideIcon /> | |||
{!developmentMode ? ( |
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.
Fixes regression introduced in #14403. We only want to display the link when not in dev mode.
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.
Made a few tweaks. Let me know your thoughts.
"Status": "Status", | ||
"The state of the plugin.": "The state of the plugin.", | ||
"Enabled": "Enabled", | ||
"A boolean to determine whether or not the plugin renders in console.": "A boolean to determine whether or not the plugin renders in console.", |
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.
What about shortening it? I think the "whether or not" is implied.
"A boolean to determine whether or not the plugin renders in console.": "A boolean to determine whether or not the plugin renders in console.", | |
"A boolean to determine if the plugin renders in console.": "A boolean to determine if the plugin renders in console.", |
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.
Sorry, @opayne1. I rewrote this while making some additions and just noticed your comment. The text now reads, Whether or not the plugin renders in console.
. WDYT?
"Enabled": "Enabled", | ||
"A boolean to determine whether or not the plugin renders in console.": "A boolean to determine whether or not the plugin renders in console.", | ||
"CSP Violations": "CSP Violations", | ||
"Plugin might have violated the Console Content Security Policy.": "Plugin might have violated the Console Content Security Policy.", |
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.
Should this one be similar to the one on line 235? or can we make it a complete sentence/
"Plugin might have violated the Console Content Security Policy.": "Plugin might have violated the Console Content Security Policy.", | |
"This plugin might have violated the Console Content Security Policy.": "This plugin might have violated the Console Content Security Policy.", |
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.
Sorry, @opayne1. I rewrote this while making some additions and just noticed your comment. The text now reads, Whether or not the plugin might have violated the Console Content Security Policy.
. WDYT?
be219d6
to
505389d
Compare
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
QE Approver: Also worth to notice that this PR is cherry-picking CONSOLE-4264: Notify users of Console plugin related Content Security Policy violations and also contributing a fix for issue found by @yapei. @opayne1 please review only commits contributed by @rhamilto, since @vojtechszocs was already reviewed by you. |
/test e2e-gcp-console |
/label px-approved |
Checked on cluster launched against the pr, tested consoleplugin 'Details' page and 'Plugin manifest' tab, features work as expected. |
@rhamilto: This pull request references CONSOLE-4269 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.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
a44b25e
to
833d56b
Compare
/label docs-approved |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, rhamilto 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 |
Hey, @rhamilto. This is great. I'm happy to see we have a nice details page. Out of scope for this PR and story, but I had a few thoughts for future improvements:
|
Thanks for the feedback, @spadgett.
Easy enough to add.
Agreed.
@vojtechszocs and I started down the path of using Monaco, but decided against it for the initial implementation because the read only mode of the existing YAML editor posed some usability questions that we didn't have time to contend with.
Easy enough to add. |
3f37ea0
into
openshift:master
@spadgett, maybe something like this (note all the details in the video are not accurate such as Screen.Recording.2024-11-19.at.9.08.21.AM.mov |
@spadgett, we must've done something wonky with our initial attempt as this appears to be working correctly now. I do wonder, is it weird to have two YAML editors? Screen.Recording.2024-11-19.at.2.52.36.PM.mov |
Hmm. Given the variance amongst the extension properties, this one is a bit trickier to tabularize. 🤔 |
@rhamilto Yeah, that looks good. We should probably show the current console version in the warning popover and add something to the plugin list table if we don't already have it. We can make it a separate story. |
Yeah, I was hesitant about that, too. But even just the list of extension IDs would be helpful. We could show the resource kind for any extension that targets a specific resource and leave the column blank for other extension points. Or maybe not a table, but some other presentation. |
Yeah, I'm undecided. Maybe we can style it differently or simplify the editor to make it more clear that it's read-only? All of these suggestions can be follow-up PRs, particularly since this has merged. I'd say the compatible versions tweak is the most important to help admins troubleshoot plugins that aren't enabled. |
Includes changes from #14374, which should merge first. Note #14374 includes a bug that needs to be addressed where not Loaded plugins do not appear in the
Console plugins
list.In collaboration with @vojtechszocs 🥇
TODOs:
Description
,Version
,Status
,Enabled
, andCSP Violations
popover text | @opayne1, can you help here?Demos
Plugin is
Loaded
Screen.Recording.2024-11-13.at.4.03.13.PM.mov
Plugin is not
Loaded
(information from pluginStore is not available)Screen.Recording.2024-11-13.at.4.08.36.PM.mov
Development setup
console-demo-plugin
locally.