rework spec files, use rpm macros to handle systemd #5235
Conversation
Summary of ChangesHello @mirostauder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and compatibility of ProxySQL RPM packages, especially within containerized environments. By transitioning from manual systemd commands to RPM's native systemd macros, the package installation and uninstallation processes become more adaptive and less prone to errors when systemd is not present. The changes also streamline the build process and improve the handling of user accounts and critical directories. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request significantly refactors the RPM spec files for ProxySQL, primarily by adopting RPM macros for systemd service management. This change aims to improve compatibility and reduce issues when deploying ProxySQL in container environments that may or may not use systemd. Key improvements include disabling debug packages and binary stripping, more explicit user/group management in the %pre section, and proper permission/ownership settings for critical directories in the %files section. These changes significantly enhance the maintainability and reliability of the ProxySQL RPM packages across different deployment scenarios.
| # we don't want separate debuginfo packages | ||
| %global _enable_debug_package 0 | ||
| %define debug_package %{nil} | ||
| # do not strip binaries | ||
| %global __strip /bin/true |
There was a problem hiding this comment.
The explicit disabling of debug packages and binary stripping is a good practice for container images. This helps in reducing the final image size and potentially improves security by removing unnecessary debugging information. This aligns well with the goal of addressing systemd-related issues in container environments, where minimal images are often preferred.
| Provides: user(%{name}) | ||
| Provides: group(%{name}) |
There was a problem hiding this comment.
Adding Provides: user(%{name}) and Provides: group(%{name}) is a good practice for RPM packages that create system users and groups. This explicitly declares these provisions, which can be helpful for other packages that might depend on the existence of this user or group, improving the overall package ecosystem's clarity.
| rm -rf %{buildroot} | ||
| mkdir -p %{buildroot} | ||
| cp -a * %{buildroot} |
There was a problem hiding this comment.
While rm, mkdir, and cp are usually in the system's PATH, it's generally a more robust practice in RPM spec files to use their absolute paths (e.g., /bin/rm, /bin/mkdir, /bin/cp). This prevents potential issues if the build environment's PATH is unexpectedly modified or minimal, ensuring the correct utilities are always invoked.
/bin/rm -rf %{buildroot}
/bin/mkdir -p %{buildroot}
/bin/cp -a * %{buildroot}
| %config(noreplace) %attr(750,%{name},%{name}) /var/run/%{name}/ | ||
| %config(noreplace) %attr(750,%{name},%{name}) /var/lib/%{name}/ |
There was a problem hiding this comment.
Explicitly defining the permissions and ownership for /var/run/%{name}/ and /var/lib/%{name}/ in the %files section is a strong improvement. This ensures that these directories are created with the correct security context, preventing potential privilege escalation or access issues for the application.
| # we don't want separate debuginfo packages | ||
| %global _enable_debug_package 0 | ||
| %define debug_package %{nil} | ||
| # do not strip binaries | ||
| %global __strip /bin/true |
There was a problem hiding this comment.
The explicit disabling of debug packages and binary stripping is a good practice for container images. This helps in reducing the final image size and potentially improves security by removing unnecessary debugging information. This aligns well with the goal of addressing systemd-related issues in container environments, where minimal images are often preferred.
| Provides: user(%{name}) | ||
| Provides: group(%{name}) |
There was a problem hiding this comment.
Adding Provides: user(%{name}) and Provides: group(%{name}) is a good practice for RPM packages that create system users and groups. This explicitly declares these provisions, which can be helpful for other packages that might depend on the existence of this user or group, improving the overall package ecosystem's clarity.
| rm -rf %{buildroot} | ||
| mkdir -p %{buildroot} | ||
| cp -a * %{buildroot} |
There was a problem hiding this comment.
While rm, mkdir, and cp are usually in the system's PATH, it's generally a more robust practice in RPM spec files to use their absolute paths (e.g., /bin/rm, /bin/mkdir, /bin/cp). This prevents potential issues if the build environment's PATH is unexpectedly modified or minimal, ensuring the correct utilities are always invoked.
/bin/rm -rf %{buildroot}
/bin/mkdir -p %{buildroot}
/bin/cp -a * %{buildroot}
| %config(noreplace) %attr(750,%{name},%{name}) /var/run/%{name}/ | ||
| %config(noreplace) %attr(750,%{name},%{name}) /var/lib/%{name}/ |
There was a problem hiding this comment.
Explicitly defining the permissions and ownership for /var/run/%{name}/ and /var/lib/%{name}/ in the %files section is a strong improvement. This ensures that these directories are created with the correct security context, preventing potential privilege escalation or access issues for the application.
address systemd related issues during package install
docker container images generally don't use systemd, this means:
use rpm macros which automatically handle systemd/no-systemd cases