-
Notifications
You must be signed in to change notification settings - Fork 32
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
Keep custom executable name if defined #207
Conversation
@@ -125,7 +127,7 @@ const config = async (env): Promise<Configuration> => ({ | |||
|
|||
output: { | |||
clean: { | |||
keep: /gpx_.*/, | |||
keep: new RegExp(`gpx_.*|${pluginJson.executable || 'gpx_'}.*`), |
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.
I'm keeping gpx_.*
for backwards compatibility and intentionally using a redundant fallback. This might be me being too cautious but there's no harm in a duplicated regex or
if this might cover some weird edge cases where you have both gpx_ prefixed binaries and custom binaries.
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 change looks good but I'm not sure it will work with nested plugins. 🤔
If an App plugin has a plugin.json
in the root src
directory but it is the datasource plugin.json
that declares the executable e.g ../gpx_zabbix-plugin
I think this will continue to fail. Should we worry much?
output.clean.keep
can also accept a function. Could we do anything smarter there to understand if the file should be removed or not.
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.
@jackw good point on nested plugins. what if Instead I just check that it doesn't have any of the target platforms suffix? e.g. amd64, arm, etc?
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.
Yeah I think that should work. From what I know we append darwin
, windows
, and linux
to whatever is set in the plugin.json executable
field. 👍
@@ -125,7 +127,7 @@ const config = async (env): Promise<Configuration> => ({ | |||
|
|||
output: { | |||
clean: { | |||
keep: /gpx_.*/, | |||
keep: new RegExp(`.*?_(amd64|arm(64)?)(.exe)?`), |
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.
@jackw I implemented the change here.
I kept the getPluginJson
method because it can be useful for future features.
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! 🚀
🚀 PR was released in |
What this PR does / why we need it:
Keeps compiled binaries that don't follow the
gpx_.*
prefix in the./dist
directory when webpack runs.Which issue(s) this PR fixes:
Fixes #204
Special notes for your reviewer:
📦 Published PR as canary version:
Canary Versions
✨ Test out this PR locally via:
npm install @grafana/create-plugin@1.2.0-canary.207.948a872.0 npm install @grafana/sign-plugin@1.1.0-canary.207.948a872.0 # or yarn add @grafana/create-plugin@1.2.0-canary.207.948a872.0 yarn add @grafana/sign-plugin@1.1.0-canary.207.948a872.0