-
Notifications
You must be signed in to change notification settings - Fork 277
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
Fix for failing OSD distribution build #3084
Conversation
helper_string=plugin_helpers | ||
else | ||
helper_string=plugin-helpers |
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.
Spacing can be fixed a bit to 4 spaces.
helper_cmd=$(jq -r '.scripts.plugin_helpers' package.json) | ||
if [ "$helper_cmd" != null ]; then | ||
helper_string=plugin_helpers | ||
else | ||
helper_string=plugin-helpers |
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.
Could you use ALL_CAPITAL for both var name?
Thanks.
Codecov Report
@@ Coverage Diff @@
## main #3084 +/- ##
=======================================
Coverage 93.17% 93.17%
=======================================
Files 167 167
Lines 4602 4602
=======================================
Hits 4288 4288
Misses 314 314 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Rishabh Singh <sngri@amazon.com>
HELPER_CMD=$(jq -r '.scripts.plugin_helpers' package.json) | ||
if [ "$HELPER_CMD" != null ]; then | ||
HELPER_STRING=plugin_helpers | ||
else | ||
HELPER_STRING=plugin-helpers | ||
fi |
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.
Is this a fast fix that otherwise all dashboards plugins needs to make a change in their package.json? Changing config during build can be dangerous.
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.
There is no consistency on all the dashboards on how to call plugin helper.
Not sure which repo starts using plugin-helpers and some follows but some dont.
At this point this is a quick fix but the permanent fix is to let everyone use plugin_helpers.
HELPER_CMD=$(jq -r '.scripts.plugin_helpers' package.json) | ||
if [ "$HELPER_CMD" != null ]; then | ||
HELPER_STRING=plugin_helpers | ||
else | ||
HELPER_STRING=plugin-helpers | ||
fi |
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.
There is no consistency on all the dashboards on how to call plugin helper.
Not sure which repo starts using plugin-helpers and some follows but some dont.
At this point this is a quick fix but the permanent fix is to let everyone use plugin_helpers.
Signed-off-by: Rishabh Singh sngri@amazon.com
Description
This PR fixes the failing distribution build for OSD and updates the manifest to use the new
dashboards-notifications
repository.Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.