CONSOLE-3883: Improve dynamic plugin docs #13637

vojtechszocs · 2024-02-26T19:22:37Z

Include PatternFly upgrade notes
Include notes on hosting plugin assets

openshift-ci-robot · 2024-02-26T19:22:58Z

@vojtechszocs: This pull request references CONSOLE-3883 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.16.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.

openshift-ci-robot · 2024-02-26T19:23:42Z

@vojtechszocs: This pull request references CONSOLE-3883 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.16.0" version, but no target version was set.

In response to this:

Include PatternFly upgrade notes

Include notes on hosting plugin assets

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.

spadgett

Thanks @vojtechszocs 👍

@opayne1 FYI

spadgett · 2024-02-26T19:40:46Z

frontend/packages/console-dynamic-plugin-sdk/README.md

+
+## Serving plugin assets
+
+Dynamic plugins are deployed on a cluster via OLM operators. Each plugin deployment should include a web


It's not required to use an OLM operator, just a good way to deploy and manage your plugin.

@spadgett Should I rephrase it differently?

I'd probably just say plugins are deployed as workloads on the cluster.

spadgett · 2024-02-26T19:42:47Z

frontend/packages/console-dynamic-plugin-sdk/README.md

+
+**Important!** Make sure to configure your plugin web server to use MIME type `text/javascript` for all
+JS assets to avoid any issues with the `X-Content-Type-Options: nosniff` security header used by Console
+when fetching plugin assets via Bridge URL `/api/plugins/<plugin-name>`.


nitpick: I'd leave off the URL path here since that's an implementation detail plugins shouldn't need to know about.

The warning is a great point and important to document. It might be worth mentioning that the plugin template will have the mime types set up properly.

Agreed, will update.

just confirming: is it alright if plugin's web server adds application/javascript by default instead of explicitly text/javascript ?? (so far it is working)...

@SanjalKatiyar Thanks, this actually led me to realize what the core issue was.

Check out https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options

Console loads dynamic plugin scripts by injecting <script> elements into the main DOM document.

Console Bridge server adds X-Content-Type-Options: nosniff HTTP response header to all plugin asset requests.

Web browsers that comply with X-Content-Type-Options: nosniff header will block <script> initiated requests when the MIME type of requested asset is not valid.

application/javascript is considered to be a legacy JavaScript MIME type but is still a valid one.

Looking further into this, I think it's OK to use a legacy MIME type like application/javascript.

Note that the proper MIME type for JavaScript according to current HTML spec is text/javascript.

The core of the issue is "always make sure to provide a valid mime type for JS assets" so that you won't end up facing errors in the browser like this one

Refused to execute script from 'http://localhost:9000/api/plugins/foo-plugin/some-script.js' because its MIME type ('') is not executable, and strict MIME type checking is enabled.

I will update the README accordingly.

spadgett · 2024-02-26T19:43:43Z

frontend/packages/console-dynamic-plugin-sdk/README.md

+
+### Caching
+
+The following table lists important plugin assets that must NOT be cached by the plugin web server.


Can they be cached if appropriate cache-control headers are returned?

Yes, AFAIK the Bridge plugin asset proxy endpoint does not do anything special in this regard. cc @jhadvig

I know @TheRealJon has fixed some caching bugs on the console side. Can we confirm it's still a limitation?

I believe we should be able to handle caching as long as it's done properly by the plugin. If this is really a restriction, we should look at adding cache-busting parameters to the console request to avoid the problem.

@spadgett Looking into this, all plugin entry scripts are loaded using ?cacheBuster query parameter to circumvent potential caching issues on the server side. This was fixed for OCP Console 4.14 and above.

Given that JSON files should be generally OK to cache (assuming we don't expect their content to change), I think I can remove the whole "Caching" section above then.

spadgett · 2024-02-26T19:44:20Z

frontend/packages/console-dynamic-plugin-sdk/README.md

+| `plugin-manifest.json` |                                                                |
+| `plugin-entry.js`      | Applies only to Console plugin SDK versions 0.0.x              |
+
+We generally recommend disabling caching of any JSON resources hosted by the plugin web server.


I'm not sure I agree with this. In particular, I'd recommend using caching for i18n message files.

I think the important thing is that caching is set up correctly and we fix any caching bugs on the console side.

OK, you have a point with the i18n resources not supposed to be changed.

I will remove this line.

spadgett · 2024-02-26T19:46:20Z

frontend/packages/console-dynamic-plugin-sdk/upgrade-PatternFly.md

+
+## Console 4.15
+
+All plugins that target OpenShift Console 4.15 should upgrade to PatternFly 5.x to take advantage


I'd clarify that this is for plugins that only target 4.15 and newer, not for plugins that want to be compatible with older versions.

Right, I will update it to be more explicit.

spadgett

/lgtm

👍

spadgett · 2024-02-28T16:57:12Z

/lgtm

jhadvig

Small nit otherwise looks good.
Docs Approver:
/assign @opayne1

jhadvig · 2024-03-05T09:27:05Z

frontend/packages/console-dynamic-plugin-sdk/upgrade-PatternFly.md

+A dynamic plugin targets one or more versions of OpenShift Console. Each Console version supports
+specific version(s) of [PatternFly](https://www.patternfly.org/) in terms of CSS styling.
+
+Check the [OpenShift Console Versions vs PatternFly Versions][console-pf-versions] table for


Lets link the table here

@jhadvig This is a reference-style link to console-pf-versions declared at the end of the Markdown document.

[console-pf-versions]: ./README.md#openshift-console-versions-vs-patternfly-versions

openshift-ci-robot · 2024-03-05T09:29:38Z

@vojtechszocs: This pull request references CONSOLE-3883 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.16.0" version, but no target version was set.

In response to this:

Include PatternFly upgrade notes

Include notes on hosting plugin assets

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.

opayne1

Just 2 small wording/grammar related comments, but otherwise looks good so I will go ahead and approve.

I have opened a docs PR to capture these updates in the user facing docs. Open to any feedback you have!

frontend/packages/console-dynamic-plugin-sdk/upgrade-PatternFly.md

opayne1 · 2024-03-05T14:51:20Z

/label docs-approved

Co-authored-by: Olivia Payne <opayne@redhat.com>

openshift-ci · 2024-03-05T17:18:39Z

New changes are detected. LGTM label has been removed.

vojtechszocs · 2024-03-05T17:20:02Z

@opayne1 Thank you for your comments, I've applied the suggested changes.

rhamilto · 2024-03-06T16:04:51Z

/lgtm
/label tide/merge-method-squash

openshift-ci · 2024-03-06T16:11:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, nicolethoen, rhamilto, spadgett, vojtechszocs

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:

~~frontend/packages/console-dynamic-plugin-sdk/OWNERS~~ [jhadvig,rhamilto,spadgett,vojtechszocs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-03-06T23:01:18Z

/retest-required

Remaining retests: 0 against base HEAD 7bfcdc2 and 2 for PR HEAD f337740 in total

rhamilto · 2024-03-07T12:43:59Z

/retest

rhamilto · 2024-03-07T13:33:18Z

/retest

vojtechszocs · 2024-03-07T15:23:55Z

/retest

openshift-ci · 2024-03-07T19:41:48Z

@vojtechszocs: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2024-03-08T11:59:13Z

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-console-container-v4.16.0-202403081048.p0.g77d383d.assembly.stream.el8 for distgit openshift-enterprise-console.
All builds following this will include this PR.

* Improve dynamic plugin docs * Add section on PF CSS styling * Address review comments * Apply suggestions from code review Co-authored-by: Olivia Payne <opayne@redhat.com> --------- Co-authored-by: Olivia Payne <opayne@redhat.com>

vojtechszocs changed the title ~~Improve dynamic plugin docs~~ CONSOLE-3883: Improve dynamic plugin docs Feb 26, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 26, 2024

openshift-ci bot requested review from christianvogt and spadgett February 26, 2024 19:27

openshift-ci bot added component/sdk Related to console-plugin-sdk approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2024

Improve dynamic plugin docs

216bff0

vojtechszocs force-pushed the improve-plugin-docs branch from 9d8f345 to 216bff0 Compare February 26, 2024 19:31

Add section on PF CSS styling

5853bc4

spadgett reviewed Feb 26, 2024

View reviewed changes

nicolethoen approved these changes Feb 28, 2024

View reviewed changes

Address review comments

bc0dd41

spadgett approved these changes Feb 28, 2024

View reviewed changes

openshift-ci bot assigned spadgett Feb 28, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2024

This was referenced Feb 29, 2024

Configure mco console plugin web server to use recommended MIME type red-hat-storage/odf-console#1242

Open

Configure odf console plugin web server to use recommended MIME type red-hat-storage/odf-operator#376

Open

jhadvig approved these changes Mar 5, 2024

View reviewed changes

jhadvig added qe-approved Signifies that QE has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Mar 5, 2024

opayne1 mentioned this pull request Mar 5, 2024

OCPBUGS#30133: Updates to dynamic plugin docs openshift/openshift-docs#72479

Merged

1 task

opayne1 reviewed Mar 5, 2024

View reviewed changes

frontend/packages/console-dynamic-plugin-sdk/upgrade-PatternFly.md Outdated Show resolved Hide resolved

frontend/packages/console-dynamic-plugin-sdk/upgrade-PatternFly.md Outdated Show resolved Hide resolved

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Mar 5, 2024

Apply suggestions from code review

f337740

Co-authored-by: Olivia Payne <opayne@redhat.com>

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2024

SanjalKatiyar mentioned this pull request Mar 6, 2024

Upgrade to Patternfly 5 red-hat-storage/odf-console#1107

Merged

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 6, 2024

rhamilto added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2024

openshift-ci bot assigned rhamilto Mar 6, 2024

openshift-merge-bot bot merged commit 77d383d into openshift:master Mar 7, 2024
6 checks passed


		## Serving plugin assets

		Dynamic plugins are deployed on a cluster via OLM operators. Each plugin deployment should include a web


		### Caching

		The following table lists important plugin assets that must NOT be cached by the plugin web server.


		## Console 4.15

		All plugins that target OpenShift Console 4.15 should upgrade to PatternFly 5.x to take advantage

CONSOLE-3883: Improve dynamic plugin docs #13637

CONSOLE-3883: Improve dynamic plugin docs #13637

Conversation

vojtechszocs commented Feb 26, 2024 • edited Loading

openshift-ci-robot commented Feb 26, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Feb 26, 2024 • edited by openshift-ci bot Loading

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment • edited Loading

Choose a reason for hiding this comment

spadgett commented Feb 28, 2024

jhadvig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 5, 2024 • edited by openshift-ci bot Loading

opayne1 left a comment

Choose a reason for hiding this comment

opayne1 commented Mar 5, 2024

openshift-ci bot commented Mar 5, 2024

vojtechszocs commented Mar 5, 2024

rhamilto commented Mar 6, 2024

openshift-ci bot commented Mar 6, 2024

openshift-ci-robot commented Mar 6, 2024

rhamilto commented Mar 7, 2024

rhamilto commented Mar 7, 2024

vojtechszocs commented Mar 7, 2024

openshift-ci bot commented Mar 7, 2024

openshift-bot commented Mar 8, 2024

vojtechszocs commented Feb 26, 2024 •

edited

Loading

openshift-ci-robot commented Feb 26, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Feb 26, 2024 •

edited by openshift-ci bot

Loading

vojtechszocs Feb 27, 2024 •

edited

Loading

spadgett left a comment •

edited

Loading

openshift-ci-robot commented Mar 5, 2024 •

edited by openshift-ci bot

Loading