Set schedule permissions #476
Conversation
WalkthroughThe changes update the link checker’s command parameters in GitHub Actions workflows and the Makefile, modifying connection limits and the User-Agent string. Additionally, the backup configuration in the example has been enhanced with a network check. Permission checks in various scheduling handlers have been refactored to use a new Changes
Sequence Diagram(s)sequenceDiagram
participant D as Developer
participant U as Unit Instance
participant FS as FileSystem (afero.Fs)
participant user as User
D->>U: NewUnit(user)
D->>U: Generate(config)
U->>U: Call GetUserDir()
U->>U: Load template (loadTemplate)
U->>FS: Write unit file
FS-->>U: File written confirmation
U-->>D: Return success
Possibly related PRs
Suggested labels
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release-doc.yml (1)
52-52: Updated broken links command parameters.The
cmd_paramsstring now increases the connection limit to 8 per host and updates the User-Agent to "Muffet/2.10.8". Please check that the inclusion of--skip-tls-verificationis intentional and that these settings meet your project's network testing requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/doc.yml(1 hunks).github/workflows/release-doc.yml(1 hunks)Makefile(1 hunks)examples/linux.yaml(1 hunks)schedule/handler_crond.go(1 hunks)schedule/handler_darwin.go(1 hunks)schedule/handler_systemd.go(6 hunks)systemd/drop_ins.go(5 hunks)systemd/generate.go(6 hunks)systemd/generate_test.go(22 hunks)systemd/read.go(2 hunks)systemd/read_test.go(7 hunks)user/user.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
systemd/generate_test.go (3)
user/user.go (1)
User(3-9)systemd/generate.go (3)
GetSystemDir(257-259)Unit(115-118)Config(93-113)schedule/config.go (1)
Config(10-27)
schedule/handler_systemd.go (4)
user/user_windows.go (1)
Current(9-23)systemd/generate.go (5)
UserUnit(70-70)SystemUnit(71-71)NewUnit(120-125)Config(93-113)Unit(115-118)schedule/permission.go (2)
PermissionFromConfig(16-30)Permission(7-7)schedule/config.go (1)
Config(10-27)
🪛 golangci-lint (1.64.8)
systemd/generate.go
234-234: Error return value of afero.Walk is not checked
(errcheck)
🪛 GitHub Check: Build and test (1.24, ubuntu-latest)
systemd/generate.go
[failure] 234-234:
Error return value of afero.Walk is not checked (errcheck)
🪛 GitHub Actions: Build
systemd/generate.go
[error] 234-234: Error return value of afero.Walk is not checked (errcheck)
🔇 Additional comments (31)
Makefile (1)
301-304: Refined checklinks target in Makefile.The updated checklinks rule uses muffet with a stricter limit of 8 connections per host and dynamically injects the muffet version into the User-Agent header using
$$(muffet --version). Please verify that this dynamic header substitution consistently yields the expected format in your CI environment, ensuring no unexpected whitespace or format issues arise..github/workflows/doc.yml (1)
58-58: Enhanced link checker invocation in workflow.The command parameters have been modified to exclude specific domains, set a fixed buffer size and timeout, and adopt the new connection and header settings. Note the addition of
--color=always—please ensure that forcing coloured output does not lead to undesired artefacts when run in a headless CI context.user/user.go (1)
11-15: Well-designed method for checking root privileges.This new
IsRoot()method centralizes the logic for determining if a user has root privileges, making the code more maintainable and consistent across the codebase. The method checks both theSudoRootflag and the user ID, covering all cases where a user might have elevated privileges.schedule/handler_darwin.go (1)
271-271: Good refactoring to use the new IsRoot() method.Replacing the direct check of
SudoRootandUid == 0with the newIsRoot()method improves code readability and ensures consistent privilege checking across the codebase.examples/linux.yaml (2)
101-101: Example drop-in configuration for systemd.This commented example demonstrates how to specify systemd drop-in files, which is helpful for users who need to customize systemd unit files.
108-108: Good addition of network check for backup scheduling.Adding
after-network-online: trueensures that backup operations only proceed when network connectivity is available, which is crucial for remote repository backups. This prevents failed backup attempts when the network is unavailable.schedule/handler_crond.go (1)
205-205: Good refactoring to use the new IsRoot() method.Replacing the direct check of
SudoRootandUid == 0with the newIsRoot()method improves code readability and ensures consistent privilege checking across the codebase.systemd/drop_ins.go (2)
26-38: Encapsulated check for timer drop-in looks fine.
No issues spotted; the method neatly switches to usingu.fs.Openwith proper error handling.
40-100: Well-structured drop-in creation method.
The usage ofu.fsfor file operations is clear and consistent. Error handling on directory creation, reading, and removal is suitably done.systemd/read.go (2)
19-59: Instance-based read method is coherent.
Migrating to a receiver method allows better encapsulation. The error propagation and validation for.servicesuffix appear correct.
63-99: Reading systemd unit content now leveragesu.fs.
Usingafero.ReadFilealigns well with the new struct-based approach. No immediate concerns noted.systemd/read_test.go (1)
10-183: Enhanced test coverage for different users and working directories.
Adopting theUnit{fs: fs, user: ...}approach is consistent with the refactoring. Parallel tests and updated environment usage show good practice.systemd/generate.go (3)
115-124: Introducing theUnitstruct is a logical design enhancement.
Bundlingfsanduserneatly clarifies ownership and file operations.
128-229: Systemd unit generation method is functionally correct.
The changes unify filesystem interaction and user details in a neat, object-oriented style. No major performance or security concerns identified.
283-301: Loading templates from the filesystem is well-structured.
The fallback to default templates iffilenameis empty is clear, and usingu.fs.Opensuits the new approach.systemd/generate_test.go (8)
16-20: Good addition of test user objects for improved test clarity.These user objects make test scenarios more explicit and provide clear differentiation between standard user, sudo user, and root user contexts, which is essential for testing systemd scheduling permissions properly.
23-24: Nice improvement with test parallelization and isolated file systems.Making tests parallel and giving each test its own isolated filesystem improves test performance and eliminates potential state interference between tests.
30-32: Good refactoring to use explicit filesystem references.The test now properly references the in-memory filesystem for assertions, making the relationship between the test filesystem and assertions clear.
33-44: Clean implementation of Unit struct usage.The change to use the Unit struct with its filesystem and user parameters encapsulates the dependencies well and follows good object-oriented design principles.
71-83: Explicit user assignment in test configuration.Good addition of the test user in the Generate call, and explicitly setting the username in the configuration. This makes the test more self-contained and explicit about its test scenario.
90-90: Clear home directory reference using test user.Using the test user's HomeDir property instead of os.UserHomeDir() makes the test more deterministic and independent of the actual system environment.
117-117: Well-structured user directory path construction.The test correctly builds the systemd user directory path based on the test user's home directory, which aligns with how systemd locates user unit files.
614-619: Good helper function refactoring for file assertions.The helper functions for file assertions are well-designed with proper error handling and descriptive failure messages, making test failures more informative.
schedule/handler_systemd.go (8)
111-112: Improved user handling with dedicated variable.Using a dedicated variable for the current user improves readability and allows for consistent user context throughout the function.
118-134: Important permission transition handling to prevent duplicate units.This crucial addition checks for and removes existing units when changing permission types (between user and system). This prevents the confusing situation where the same job might have multiple units with different permissions.
The PR objective is well-implemented here, handling the scenario where permission changes from user to system or vice versa. This code ensures clean transitions between unit types, which is especially important with the new
user_logged_onpermission.
136-137: Good refactoring to use the Unit abstraction.Using
systemd.NewUnit(u)leverages the new Unit struct abstraction, which properly encapsulates filesystem and user context for systemd operations.
192-194: Consistent user handling in RemoveJob.Good consistency in user handling between CreateJob and RemoveJob methods, using the same pattern of obtaining the current user and determining the unit type based on permissions.
220-220: Proper use of Unit's GetUserDir method.Using the unit's GetUserDir method instead of a direct systemd call follows the encapsulation pattern established by the Unit struct.
266-271: Sensible handling of root user with user units.This special case handles a limitation in journalctl where it doesn't accept the "-M user@" parameter when running as root. The warning message helps users understand why they might not see logs.
324-324: Simplified permission check using IsRoot.Using the user.IsRoot() method instead of the more verbose condition improves code clarity and maintainability.
506-506: Consistent Unit usage in config reading.Using systemd.NewUnit(user.Current()).Read() follows the same pattern as elsewhere in the code, ensuring consistent handling of user context when reading unit configurations.
systemd/generate.go
Outdated
| afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _ = u.fs.Chown(path, u.user.Uid, u.user.Gid) | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Handle the return value from afero.Walk to prevent pipeline failures.
Currently, the returned error is ignored, causing a lint and build failure. A simple fix is to check this error and propagate it:
- afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error {
+ if err := afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error {
if err != nil {
return err
}
_ = u.fs.Chown(path, u.user.Uid, u.user.Gid)
return nil
- })
+ }); err != nil {
+ return err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| _ = u.fs.Chown(path, u.user.Uid, u.user.Gid) | |
| return nil | |
| }) | |
| if err := afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| _ = u.fs.Chown(path, u.user.Uid, u.user.Gid) | |
| return nil | |
| }); err != nil { | |
| return err | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
234-234: Error return value of afero.Walk is not checked
(errcheck)
🪛 GitHub Check: Build and test (1.24, ubuntu-latest)
[failure] 234-234:
Error return value of afero.Walk is not checked (errcheck)
🪛 GitHub Actions: Build
[error] 234-234: Error return value of afero.Walk is not checked (errcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
systemd/generate.go (1)
234-240:⚠️ Potential issueMissing error handling for
afero.WalkThe error returned from the
afero.Walkfunction is not checked, which could lead to silent failures during directory traversal.Apply this fix:
- _ = afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error { + if err := afero.Walk(u.fs, dropInDir, func(path string, info fs.FileInfo, err error) error { if err != nil { return err } _ = u.fs.Chown(path, u.user.Uid, u.user.Gid) return nil - }) + }); err != nil { + return err + }
🧹 Nitpick comments (1)
systemd/generate.go (1)
198-198: Consider logging Chown errorsThe code ignores errors from
u.fs.Chowncalls. While these errors might not be critical enough to halt execution, logging them would provide useful debugging information.- _ = u.fs.Chown(filePathName, u.user.Uid, u.user.Gid) + if err := u.fs.Chown(filePathName, u.user.Uid, u.user.Gid); err != nil { + clog.Debugf("failed to change ownership of %s: %v", filePathName, err) + }Also applies to: 220-220, 238-238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
systemd/generate.go(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
systemd/generate.go (3)
user/user.go (1)
User(3-9)schedule/config.go (1)
Config(10-27)util/collect/collect.go (2)
All(7-14)Not(17-20)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and test (1.24, windows-latest)
🔇 Additional comments (9)
systemd/generate.go (9)
115-118: Good structuring with the new Unit typeThe introduction of a
Unitstruct encapsulating filesystem operations and user information is a solid refactoring. This improves testability and maintainability by reducing reliance on global state.
120-125: Constructor follows Go idiomatic patternsThe
NewUnitconstructor properly initializes the struct with the OS filesystem and user information. This is the recommended pattern for creating new instances in Go.
142-152: Environment variable handling improvedThe environment handling is now more explicit and controlled. Setting HOME based on the user's home directory ensures consistent behaviour across different execution contexts.
196-199: File ownership management for user unitsThe file ownership management when creating user units with sudo privileges is a good security practice, ensuring that the files are owned by the original user rather than root.
Also applies to: 218-221, 232-241
224-225: Usage of collect package is elegantUsing the
collectpackage with its generic functions for filtering drop-in files is a clean, functional approach that improves code readability.
248-254: GetUserDir method refactoringMoving
GetUserDirto a method onUnitaligns with the overall refactoring pattern and allows it to use the Unit's internal state for user directory resolution.
257-259: Simple accessor for system directoryThe new
GetSystemDirfunction provides a clean interface for accessing the system directory, consistent with the package's design.
283-300: Template loading encapsulationRefactoring
loadTemplateas a method onUnitaligns with the overall design approach and enables cleaner testing through dependency injection.
229-229:Details
✅ Verification successful
Missing createDropIns method definition
The code calls
u.createDropInsbut I don't see its definition in this file. Ensure this method is properly defined elsewhere in the package.Run this script to verify the method exists:
🏁 Script executed:
#!/bin/bash # Search for the createDropIns method definition rg -i --type go "func \(\w+ Unit\) createDropIns" --no-filename echo "-----" # Check if the method is used in other files rg -i --type go "createDropIns" --no-filenameLength of output: 323
Verified: The createDropIns method is defined
Upon verification, I confirmed that the
createDropInsmethod is indeed defined as shown below:func (u Unit) createDropIns(dir string, files []string) error {No further action is required.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (49.46%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #476 +/- ##
==========================================
- Coverage 79.46% 79.22% -0.24%
==========================================
Files 132 133 +1
Lines 13169 13204 +35
==========================================
- Hits 10464 10460 -4
- Misses 2292 2326 +34
- Partials 413 418 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Remove existing
systemdschedule when the unit type differs (user->systemorsystem->user)This is because of the introduction of the
user_logged_onpermission forsystemdscheduling.