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
Bug 2020428: Adapt webpack 5 related code in dynamic plugin SDK #10433
Bug 2020428: Adapt webpack 5 related code in dynamic plugin SDK #10433
Conversation
f8132b6
to
f5eb2da
Compare
@vojtechszocs: This pull request references Bugzilla bug 2020428, which is invalid:
Comment 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 kubernetes/test-infra repository. |
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.
We might need to revisit some of the assumptions we're making about shared modules down the road, but I think this is good for now.
expectSameValues(Object.keys(entryModule.init.mock.calls[0][0]), sharedPluginModules); | ||
}); | ||
|
||
it('supports plugins built with an older version of plugin SDK', () => { |
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.
👍
shared: sharedPluginModules, | ||
}).apply(compiler); | ||
const logger = compiler.getInfrastructureLogger(ConsoleRemotePlugin.name); | ||
const publicPath = `/api/plugins/${this.pkg.consolePlugin.name}/`; |
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.
Console has an option for a base context root, although we don't use it for OpenShift. Would this break that?
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.
This option means that all plugin assets will be loaded from <ConsoleOrigin>/api/plugins/<PluginName>
so relative to Console application's protocol / domain / port at runtime. Not sure if that is safe enough in relation to changing base context root 😕
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.
No, that will break context roots. We might need it for HAC as well :/
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.
frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleAssetPlugin.ts
Outdated
Show resolved
Hide resolved
Updated PR to also fix #9198 |
@vojtechszocs: This pull request references Bugzilla bug 2020428, which is invalid:
Comment 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 kubernetes/test-infra repository. |
/bugzilla refresh |
@spadgett: This pull request references Bugzilla bug 2020428, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 kubernetes/test-infra repository. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest-required Please review the full test history for this PR and help us cut down flakes. |
@vojtechszocs: All pull requests linked via external trackers have merged: Bugzilla bug 2020428 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
This PR addresses all TODO items and refactors plugin SDK webpack code to comply with webpack 5 APIs.
Closes #9198
Improvements
ConsoleAssetPlugin
is now responsible for generating and/or post-processing all Console plugin assets.ExtensionValidator
is now executed viaemit
hook. Plugins that wish to avoid emitting assets entirely due to webpack error(s) should do so viaoptimization.emitOnErrors
option.ExtensionValidator
can be disabled by passingCONSOLE_PLUGIN_SKIP_EXT_VALIDATOR=true
env. variable to your webpack command, for example:output.publicPath
option.ConsoleRemotePlugin
will always set its value to/api/plugins/<ConsolePluginName>/
. A warning will be issued in case it's specified explicitly, for example:exposed-navPage-chunk.js
Shared module configuration
*
semver range to ensure plugins always use the Console-provided shared module version