-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add dashboards rpm assemble support in opensearch-build repo #1852
Add dashboards rpm assemble support in opensearch-build repo #1852
Conversation
Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
…-rpm-dashboards Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
@spotrh could you help check the dashboards spec file again. |
Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Codecov Report
@@ Coverage Diff @@
## main #1852 +/- ##
============================================
- Coverage 94.53% 94.40% -0.14%
Complexity 19 19
============================================
Files 178 178
Lines 3625 3627 +2
Branches 27 27
============================================
- Hits 3427 3424 -3
- Misses 194 199 +5
Partials 4 4
Continue to review full report at Codecov.
|
Do you plan to fix the "missing starting space in comment"? |
That is straight up copy directly from OpenSearch core and it is expected. |
shutil.move(f"{min_bin_env_path}.backup", min_bin_env_path) | ||
if os.path.exists(f"{min_bin_env_path}.backup"): | ||
logging.info(f"Restore {min_bin_env_path}.backup to {min_bin_env_path}") | ||
shutil.move(f"{min_bin_env_path}.backup", min_bin_env_path) |
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 looks hacky, why is any of this necessary? Why is it undoing something done in extract?
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 was in for the opensearch only change.
The reason being there is a hardcoded source /etc/sysconfig/opensearch
line specifically in rpm version of core.
This caused the failure during installing of plugin zips.
In extract block we comment out the line and back up the original file.
Here we just move the backup back.
The change for dashboards is just to check if the file exist, if not then ignore this step.
Thanks.
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.
Why isn't that line removed in the OpenSearch repo in the first place, why is this happening here?
I'll let it slide, but this is a hack on top of a hack.
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.
Improvement issue: #1864
Why not fix it everywhere so that the YML look cleaner? Can be another PR. Why are we copy-pasting an entire file? Doesn't this duplicate the original source of the documentation for it? |
Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Looks good to me without the /tmp permission item in the scriptlet. |
OpenSearch core has an empty file of opensearch_dashboards.yml. This just seems like a good way for user to have a template instead of finding the settings themselves. |
Thanks @spotrh I have removed the /tmp line now :) |
Description
Add dashboards rpm assemble support in opensearch-build repo
Issues Resolved
#1545
Check List
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.