Updating submodule packages to drop-ins quadlet files #700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yarboa
requested review from
rhatdan,
lsm5,
dougsland,
sandrobonazzola,
nsednev,
aesteve-rh,
pengshanyu and
kleinffm
as code owners
|
Delete old files under
|
Yarboa
force-pushed
the
move-quadlets
branch
3 times, most recently
from
b51fd71 to
0238dbd
Compare
Yarboa
force-pushed
the
move-quadlets
branch
2 times, most recently
from
5b52ae8 to
b5c2324
Compare
resolves containers#674 During latest changes all subpackages use drop-in qudlet files Under QM etc/containers/systemd/qm.container.d/ dir Qm nested container quadlets moved to subsystems/ dir Update to quadlet files Signed-off-by: Yariv Rachmani <[email protected]>
It seems that subpackaged rpm was not set correctly Make subpackages creation could be done with the following command make TARGETS=kvm subpackages Signed-off-by: Yariv Rachmani <[email protected]>
make TARGETS=windowmanager subpackages Adding missing qm qualets Signed-off-by: Yariv Rachmani <[email protected]>
Signed-off-by: Yariv Rachmani <[email protected]>
Signed-off-by: Yariv Rachmani <[email protected]>
make TARGETS=dvb subpackages make TARGETS=img_tmpdir subpackages make TARGETS=input subpackages make TARGETS=radio subpackages make TARGETS=tesxt2speech subpackages make TARGETS=tty7 subpackages make TARGETS=ttyUSB0 subpackages Signed-off-by: Yariv Rachmani <[email protected]>
Yarboa
force-pushed
the
move-quadlets
branch
from
b5c2324 to
43ba528
Compare
Reviewer's Guide by SourceryThis pull request refactors and standardizes the handling of submodule drop‐in configuration files by moving them to the new quadlet file structure under the systemd directory, updating spec files and Makefiles with consistent RPM build variables and version patterns, and adjusting configuration file formats to use the new [Container] syntax. It also updates the documentation and build commands accordingly. Refactored Makefile Structure for Subpackages (Class Diagram)classDiagram
class MakefileBase {
+RPM_TOPDIR : string
+VERSION : string
+ROOTDIR : string
+dist() void
}
class SubpackageMakefile {
+SPECFILE : string
+target() void
}
MakefileBase <|-- SubpackageMakefile
note for MakefileBase "Common variables and methods used across all subsystem Makefiles."
note for SubpackageMakefile "Each subsystem (kvm, sound, video, etc.) defines its SPECFILE and build targets based on the standardized variables."
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Yarboa - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider renaming the 'dvp' target in the Makefile to 'dvb' to match the subsystem name and maintain consistency.
- Ensure that all Makefile targets are correctly updated in the README examples to prevent confusion for users following the instructions.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@dougsland did not finish to verify all builds. |
Did you verify all bot suggestions make sense or not? |
Moving to draft again |
Yarboa
force-pushed
the
move-quadlets
branch
from
43ba528 to
628e5bd
Compare
There was a problem hiding this comment.
Hey @Yarboa - I've reviewed your changes - here's some feedback:
Overall Comments:
- The Makefiles are becoming more consistent, but there are still some inconsistencies in variable naming and usage, especially around the version and source archive names.
- The spec files are inconsistent in whether they include the 'qm-' prefix in the Source0 URL.
- Consider using a common macro or function to install drop-in configurations to avoid repetition in the spec files.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .PHONY: subpackages | ||
| subpackages: $(TARGETS) | ||
| $(TARGETS): | ||
| @echo "Entering directory: subsystem/$@" |
There was a problem hiding this comment.
suggestion (bug_risk): Mismatch in directory naming in subpackages target
The echo message refers to 'subsystem/$@', but the actual directory path used is 'subsystems/$@'. For clarity, update the message to match the directory structure.
Suggested change
| @echo "Entering directory: subsystem/$@" | |
| @echo "Entering directory: subsystems/$@" |
There was a problem hiding this comment.
@Yarboa looks sane this comment.
Yarboa
force-pushed
the
move-quadlets
branch
from
628e5bd to
30c1cb7
Compare
make TARGETS=ros2 subpackages Adding github workflow for subpackages renaming rpm/ros2/rolling/ros2_rolling.spec -> rpm/ros2/ros2_rolling.spec Adding fixes based on tests added Signed-off-by: Yariv Rachmani <[email protected]>
Yarboa
force-pushed
the
move-quadlets
branch
from
30c1cb7 to
5fe959f
Compare
dougsland
reviewed
Feb 15, 2025
| Mount=type=bind,source=/dev/tty5,target=/dev/tty5 | ||
| Mount=type=bind,source=/dev/tty6,target=/dev/tty6 | ||
| Mount=type=bind,source=/dev/tty7,target=/dev/tty7 | ||
| Mount=type=bind,source=/dev/tty0,target=/dev/tty0 |
|
@Yarboa please address the minor comments in different patch. Also update our documentation in the main README.md |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #674
During latest changes:
Summary by Sourcery
Update submodule packages to use drop-in quadlet files for configuration, moving them under the 'etc/containers/systemd/qm.container.d/' directory. Refactor Makefiles to standardize the build process and update documentation to reflect these changes.
Enhancements:
Build:
Documentation: