Group schedule #418
Group schedule #418
Conversation
WalkthroughThe pull request introduces a series of changes to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 15
🧹 Outside diff range and nitpick comments (13)
config/group.go (1)
48-59: Consider a more dynamic approach for SchedulableCommands().The
SchedulableCommands()method is correctly implemented and provides the necessary functionality. However, the use of a hardcoded list might lead to maintenance issues if new commands are added in the future.Consider implementing a more dynamic approach, as suggested by the commented-out line:
return NewProfile(g.config, "").SchedulableCommands()This would ensure that the list of schedulable commands remains up-to-date without manual intervention.
docs/content/schedules/commands.md (3)
13-14: Approved with a suggestion for clarity.The updated description effectively explains how commands operate on profiles and groups across different versions. To further enhance clarity, consider adding a brief explanation of what a "group" is in this context, as it might not be immediately clear to new users.
Consider adding a brief explanation of "group" in this context, perhaps in parentheses after its first mention.
39-41: Valuable tip about state management.This tip provides crucial information about resticprofile's lack of state management for schedules. The recommendation to unschedule everything before making profile changes is excellent advice that can help users avoid potential issues.
Consider adding a brief example command for unscheduling everything (e.g.,
resticprofile unschedule --all) to make it even easier for users to follow this advice.🧰 Tools
🪛 LanguageTool
[grammar] ~40-~40: Collocation: With the noun “changes”, you should use the verb “make”.
Context: ...and unschedule commands. If you need to do many changes in your profiles (like mov...(MAKE_A_CHANGE)
50-51: Approved with minor grammatical suggestion.The added information about the limitations of the
--allflag with the unschedule command is valuable. It effectively reinforces the earlier tip about resticprofile not keeping state of schedules.Consider rephrasing the sentence for better flow:
-Please note using the `--all` flag won't remove schedules on deleted (or renamed) profiles. **resticprofile does not keep a state of the schedules.** +Please note that using the `--all` flag won't remove schedules on deleted (or renamed) profiles, as **resticprofile does not keep a state of the schedules.**🧰 Tools
🪛 LanguageTool
[uncategorized] ~50-~50: Possible missing comma found.
Context: ...e selected profile or profiles. Please note using the--allflag won't remove sch...(AI_HYDRA_LEO_MISSING_COMMA)
docs/content/_index.md (1)
37-37: Excellent addition of new feature information.The new line effectively communicates the latest feature addition for version v0.29.0. It follows the established format and provides crucial information about version compatibility.
To enhance clarity, consider adding a brief explanation or link to documentation about configuration v2, as some users might not be familiar with the differences between configuration versions.
README.md (1)
43-44: Excellent addition of new features!The new features for versions 0.24.0 and 0.29.0 are clearly stated and provide valuable information to users. Well done on keeping the README up-to-date with the latest enhancements.
Consider adding a brief explanation or link to documentation about "configuration
v2" for the v0.29.0 feature. This would help users understand what it means and how to use it.examples/dev.yaml (2)
Line range hint
201-201: Approve the addition of syslog logging for scheduled backups.The addition of
schedule-log: "syslog://localhost:5514"for the backup operation in the 'syslog' group is a good step towards centralised logging. This can improve log management and analysis capabilities.Consider the following points to enhance this implementation:
- For distributed systems, you might want to use a remote syslog server instead of localhost.
- Ensure that the syslog server is properly secured, especially if sensitive information is being logged.
- Verify that the chosen port (5514) doesn't conflict with other services and is allowed by any firewalls in place.
- Consider adding TLS encryption for the syslog connection if security is a concern.
Line range hint
226-226: Approve the addition of a status file on RAMDisk for the 'prom' group.The addition of
status-file: /Volumes/RAMDisk/status.jsonfor the 'prom' group is a good choice for improving I/O performance for frequently updated status information.Consider the following points to enhance this implementation:
- Ensure that your system can handle the loss of this status information on restart, as RAMDisk is volatile.
- Implement a mechanism to persist this data periodically to non-volatile storage if long-term status tracking is required.
- Verify that the RAMDisk has sufficient space to accommodate the status file, even under peak load conditions.
- Consider using a memory-mapped file on SSD as an alternative if you need a balance between performance and persistence.
wrapper_test.go (1)
1732-1746: Consider enhancing the TestRunUnlock functionThe test function is a good start, but there are a few areas where it could be improved:
- The
ctx.commandis set toconstants.CommandForget, which seems unrelated to the unlock command being tested. Consider changing this toconstants.CommandUnlockfor clarity.- The
ctx.request.argumentsare not used in the test. If they're not needed, consider removing them to avoid confusion.- The test could be more comprehensive. Consider adding assertions for other properties of the prepared command, such as the command definition or environment variables.
Here's a suggested improvement:
func TestRunUnlock(t *testing.T) { t.Parallel() profile := config.NewProfile(&config.Config{}, "TestProfile") ctx := &Context{ binary: "restic", profile: profile, - command: constants.CommandForget, - request: Request{arguments: []string{"some-string", "--some-flag", "-n"}}, + command: constants.CommandUnlock, } wrapper := newResticWrapper(ctx) args := profile.GetCommandFlags(constants.CommandUnlock) cmd := wrapper.prepareCommand(constants.CommandUnlock, args, false) assert.Equal(t, []string{"unlock"}, cmd.args) + assert.Equal(t, "restic", cmd.command) + assert.Empty(t, cmd.env) }commands_schedule.go (1)
250-264: Validate the schedule name format more robustly inpreRunScheduleThe function expects the schedule name in the format
<command>@<profile-or-group-name>. If the input does not match this format, the error message may not provide sufficient guidance.Consider enhancing the error handling to provide clearer feedback:
commandName, profileName, ok := strings.Cut(scheduleName, "@") if !ok || commandName == "" || profileName == "" { - return errors.New("the expected format of the schedule name is <command>@<profile-or-group-name>") + return fmt.Errorf("invalid schedule name '%s'. Expected format: <command>@<profile-or-group-name>", scheduleName) }This adjustment ensures users receive a precise error message, helping them correct the input format.
commands.go (1)
96-99: Ensure Consistency in Command DescriptionsIn the
longDescriptionof theschedulecommand, it mentions "the selected profile (or of all profiles)" but does not include groups. Since thedescriptionhas been updated to include "profile or group (or of all profiles and groups)", consider updating thelongDescriptionto include groups for consistency.Apply this diff to update the
longDescription:-The "schedule" command registers declared schedules of the selected profile (or of all profiles) as scheduled jobs within the scheduling service of the operating system +The "schedule" command registers declared schedules of the selected profile or group (or of all profiles and groups) as scheduled jobs within the scheduling service of the operating systemcommands_test.go (2)
94-95: Useassert.ErrorIsfor error assertionsTo improve error handling consistency, replace
assert.EqualErrorwithassert.ErrorIswhen checking for specific error types.Apply the following change:
_, _, notFoundErr := getProfileScheduleJobs(cfg, commandLineFlags{name: "non-existent"}) - assert.EqualError(t, notFoundErr, "profile 'non-existent' not found") + assert.ErrorIs(t, notFoundErr, config.ErrNotFound)
100-103: Useassert.ErrorIsfor better error specificityFor more precise error checking, consider using
assert.ErrorIsinstead ofassert.EqualError.Apply the following change:
err = requireScheduleJobs(schedules, flags) - assert.EqualError(t, err, "no schedule found for profile 'other'") + assert.ErrorIs(t, err, config.ErrNoScheduleFound)Ensure that
config.ErrNoScheduleFoundis the appropriate error constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- Makefile (1 hunks)
- README.md (2 hunks)
- commands.go (4 hunks)
- commands_schedule.go (1 hunks)
- commands_test.go (5 hunks)
- config/config.go (2 hunks)
- config/group.go (3 hunks)
- config/group_test.go (1 hunks)
- config/profile.go (2 hunks)
- config/profile_test.go (1 hunks)
- config/schedule.go (4 hunks)
- config/schedule_test.go (2 hunks)
- constants/schedule.go (1 hunks)
- deprecation.go (1 hunks)
- docs/content/_index.md (1 hunks)
- docs/content/schedules/commands.md (3 hunks)
- examples/dev.yaml (1 hunks)
- examples/dynamic.yaml (1 hunks)
- run_profile.go (1 hunks)
- wrapper_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- constants/schedule.go
🧰 Additional context used
🪛 LanguageTool
docs/content/schedules/commands.md
[grammar] ~40-~40: Collocation: With the noun “changes”, you should use the verb “make”.
Context: ...and unschedule commands. If you need to do many changes in your profiles (like mov...(MAKE_A_CHANGE)
[uncategorized] ~50-~50: Possible missing comma found.
Context: ...e selected profile or profiles. Please note using the--allflag won't remove sch...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (38)
config/group_test.go (1)
3-7: Imports look goodThe imports are appropriate for a test file. The use of
testify/assertis a good practice for writing clear and concise test assertions.deprecation.go (2)
8-8: Function comment looks good.The comment accurately describes the function's purpose and reflects its ability to handle both profiles and groups.
9-9: Function signature change looks good, but verify usage.The change to accept
anytype increases flexibility. However, ensure all calls to this function have been updated accordingly throughout the codebase.Please run the following script to verify the function usage:
✅ Verification successful
Function signature change verified successfully.
All usages of
displayDeprecationNoticeshave been updated to match the new signature. No instances of the old function namedisplayProfileDeprecationNoticeswere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `displayDeprecationNotices` match the new signature. # Test: Search for the old function name. Expect: No results. rg --type go "displayProfileDeprecationNotices" # Test: Search for the new function name. Expect: Only occurrences with `any` type argument. rg --type go -A 1 "displayDeprecationNotices"Length of output: 668
examples/dynamic.yaml (2)
3-6: Improved handling of the$databasesvariableThe new implementation for initialising the
$databasesvariable is more robust and flexible. It handles cases where the.Env.databasesenvironment variable might not be set, which prevents potential errors in the subsequent YAML configuration.This change improves the reliability of the configuration, especially in environments where the database list might not always be available.
Line range hint
15-35: Verified: Changes are compatible with existing configurationI've reviewed the usage of
$databasesin thegroupsandprofilessections. The existing range loops remain compatible with the new initialization method:
- If
.Env.databasesis not set, the loops will not execute, preventing the creation of empty or invalid configurations.- If
.Env.databasesis set, the loops will function as before, creating the necessary group and profile entries.This implementation ensures that the configuration remains flexible and error-resistant, regardless of whether the database list is provided.
config/group.go (4)
3-6: LGTM: New import added for constants package.The addition of the
constantspackage import is appropriate as it's utilised in the newly added methods.
15-15: Improved field description for CommandSchedules.The updated description for the
CommandSchedulesfield now explicitly lists the commands that can be scheduled (backup, copy, check, forget, prune). This change enhances clarity and aligns well with the newly addedSchedulableCommands()method.
61-63: LGTM: Kind() method implementation.The
Kind()method is correctly implemented, returning the appropriate constant value. This method likely fulfils a requirement of theSchedulableinterface.
Line range hint
1-64: Overall assessment: Good improvements with minor enhancement suggestion.The changes to this file enhance the
Groupstruct by adding scheduling capabilities and improving documentation. The new methodsSchedulableCommands()andKind()implement theSchedulableinterface, which is a good design choice for consistency across different schedulable entities.The only suggestion for improvement is to consider a more dynamic approach for
SchedulableCommands()to ensure easier maintenance in the future.These changes align well with the PR objective of implementing group scheduling functionality.
docs/content/schedules/commands.md (2)
4-4: LGTM: Tag update is appropriate.The addition of the "v0.29.0" tag accurately reflects the introduction of new features in this version, helping users identify when specific functionality was introduced.
15-19: Excellent addition of version change notice.This warning notice effectively communicates the important changes in group scheduling behaviour between versions. It's crucial information for users upgrading from earlier versions, and the use of the warning style appropriately draws attention to this change.
README.md (2)
64-68: Great initiative to gather user feedback!The addition of the Survey section is an excellent way to engage with your users and gather valuable feedback. The section is well-placed and clearly formatted.
Well done on providing a direct link to the survey. This makes it easy for users to participate and share their thoughts on the most important features.
63-63: Good formatting adjustmentThe addition of a blank line before the new Survey section improves the overall readability of the document. This change adheres to good markdown formatting practices.
run_profile.go (2)
Line range hint
1-1: Request clarification on missing signal handling changesThe AI-generated summary mentions new signal handling mechanisms in the
runProfilefunction, including setup for listening to interrupt signals (such asos.Interrupt,syscall.SIGTERM, andsyscall.SIGABRT). However, these changes are not visible in the provided code diff.Could you please clarify if these signal handling changes have been implemented? If so, please provide the updated code for review. These changes are important for the robustness of the application and its ability to handle termination signals gracefully.
To verify the presence of these changes, you can run the following command:
#!/bin/bash # Search for signal handling code in the runProfile function rg -A 10 "func runProfile" --type go | rg "signal.Notify|os.Interrupt|syscall.SIGTERM|syscall.SIGABRT"If this command returns results showing the signal handling code, please include this section in the code for review.
132-132: Approve function name change and suggest verificationThe change from
displayProfileDeprecationNoticestodisplayDeprecationNoticesimproves code readability without altering functionality. This simplification is a positive step towards cleaner code.To ensure consistency across the codebase, please verify that all calls to this function have been updated. You can use the following command to check for any remaining instances of the old function name:
If this command returns any results, those occurrences should be updated to use the new function name.
examples/dev.yaml (1)
137-137: Approve the addition of background scheduling for the forget operation.The addition of
schedule-priority: backgroundfor the forget operation in the 'self' group is a sensible change. Running the forget operation in the background can help optimise system resource usage.To ensure this change doesn't adversely affect system performance or other scheduled tasks, please verify:
- The forget operation completes successfully when run in the background.
- There are no conflicts with other scheduled tasks in the 'self' group.
- The overall system performance isn't negatively impacted during the forget operation.
Makefile (4)
76-77: Improved eget installation processThe changes to the eget installation process are beneficial:
- Removing the line break enhances readability.
- Adding the eget execution command ensures proper installation in the GOBIN directory.
These modifications streamline the setup process and improve reliability.
Line range hint
79-89: Standardised tool installation using egetThe adoption of eget for installing goreleaser, github-markdown-toc.go, and mockery is a positive change:
- It provides consistency across tool installations.
- The '--upgrade-only' flag prevents unnecessary reinstallations.
- The '--to' flag ensures all tools are installed in the same directory.
This approach simplifies maintenance and ensures a more uniform development environment across the team.
Line range hint
91-150: Consistent build practices maintainedThe build targets for various operating systems have been kept consistent, which is commendable:
- The use of build flags and commit information in binary builds is maintained.
- The streamlined commands ensure uniformity across different build targets.
This consistency aids in maintaining a reliable build process across different platforms.
Line range hint
1-150: Overall improvements to the MakefileThe changes to the Makefile have resulted in several improvements:
- Enhanced installation process for eget and other tools.
- Standardised tool installation using eget.
- Maintained consistency in build practices across different platforms.
These modifications contribute to a more streamlined and maintainable development process. Well done!
config/schedule_test.go (2)
349-349: Approve the priority change and request clarification.The change from "background" to "standard" as the default priority for schedules has been noted. This modification could potentially impact how scheduled tasks are executed and system resource allocation.
Could you please provide more context on the rationale behind this change? It would be helpful to understand:
- The reasoning for switching from "background" to "standard" priority.
- Any potential impacts on system performance or task execution order.
- Whether this change necessitates updates to documentation or user guidelines.
To verify the impact of this change, please run the following script to check for any other occurrences of "background" priority that might need updating:
336-336: Approve the updated error message and suggest consistency check.The change in the expected panic message looks appropriate. It removes the "g:" prefix from the profile name, which may reflect an update in how profile names are handled or displayed in error messages.
To ensure consistency across the codebase, please run the following script to check for any other occurrences of similar error messages that might need updating:
✅ Verification successful
Verified the updated error message across the codebase. Only relevant instances in
config/schedule.goandconfig/schedule_test.gowere found. No further changes are necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for similar error messages that might need updating # Test: Search for error messages related to newScheduleForProfile rg --type go "invalid use of newScheduleForProfile"Length of output: 859
Script:
#!/bin/bash # Description: Search for occurrences of the specific error message in Go files # Search for the exact error message related to newScheduleForProfile rg --type go "invalid use of newScheduleForProfile"Length of output: 286
Script:
#!/bin/bash # Description: Re-verify occurrences of the specific error message in Go files # Search for the exact error message related to newScheduleForProfile rg --type go "invalid use of newScheduleForProfile"Length of output: 286
config/schedule.go (4)
54-54: Default priority consistently updated inscheduleBaseConfigDefaultsThe change in the
Priorityfield's default value from "background" to "standard" in thescheduleBaseConfigDefaultsvariable is consistent with the modification made in theScheduleBaseConfigstruct. This ensures that default values are aligned throughout the codebase.
253-254:⚠️ Potential issueNew method
Kind()added toSchedulableinterfaceA new method
Kind() stringhas been added to theSchedulableinterface. This addition allows for retrieving the kind of schedule origin (profile or group), enhancing the interface's capabilities.Consider the following:
- Ensure all existing implementations of the
Schedulableinterface are updated to include this new method.- Update relevant documentation to reflect this new capability.
- Consider adding unit tests to verify the behaviour of this new method in various implementations.
- Verify that the return values of this method are consistently used throughout the codebase.
To identify existing implementations that need updating and to check for consistency in usage, run the following script:
#!/bin/bash # Search for existing implementations of the Schedulable interface rg --type go 'type.*struct.*{.*}.*Schedulable' -C 5 # Search for uses of the Kind() method rg --type go '\.Kind\(\)' -C 3
40-40:⚠️ Potential issueDefault priority changed from "background" to "standard"
The default value for the
Priorityfield has been modified from "background" to "standard". This change may impact the execution behaviour of scheduled tasks, potentially affecting system resource allocation and user experience.Consider the following:
- Ensure this change is intentional and aligns with the project's goals.
- Update any documentation or user guides to reflect this new default behaviour.
- Verify that existing schedules aren't adversely affected by this change.
To assess the impact of this change, please run the following script:
✅ Verification successful
Priority Default Change Verified
The default value for the
Priorityfield has been successfully updated from"background"to"standard". The search confirmed that only test files explicitly setPriorityto"background", ensuring that production code defaults are now"standard".Next Steps:
- Review
systemd/generate_test.goto ensure that the explicit"background"priority is intentional and aligns with test expectations.- Update test documentation if necessary to reflect the change in default priority behaviour.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing uses of Priority in schedules rg --type go 'Priority.*background' -C 3Length of output: 8963
Script:
#!/bin/bash # Corrected search for Priority field set to "background" in Go files rg --type go 'Priority\s*:\s*"background"' -C 3Length of output: 940
251-252:⚠️ Potential issueNew method
SchedulableCommands()added toSchedulableinterfaceA new method
SchedulableCommands() []stringhas been added to theSchedulableinterface. This addition enhances the interface's capabilities by allowing the retrieval of a list of commands that can be scheduled.Consider the following:
- Ensure all existing implementations of the
Schedulableinterface are updated to include this new method.- Update relevant documentation to reflect this new capability.
- Consider adding unit tests to verify the behaviour of this new method in various implementations.
To identify existing implementations that need updating, run the following script:
config/config.go (1)
536-536: Excellent use of predefined error constantThe change from a formatted error string to the predefined
ErrNotFoundconstant is a good improvement. This approach allows for more precise error handling by callers of theGetProfileGroupmethod, as they can now use errors.Is() to check for this specific error condition.This change enhances the API's consistency and makes error handling more robust for users of this package.
config/profile.go (2)
972-974: LGTM: New Kind() method addedThe addition of the
Kind()method to theProfilestruct is straightforward and appears to be implementing an interface or providing a way to identify the type of the profile. The implementation looks correct and consistent with the rest of the codebase.
975-975: Verify consistency of Schedules() method commentA comment has been added to the
Schedules()method indicating that schedules are only applicable to v1 configurations. While this clarification is helpful, please ensure that this behaviour is consistent with the overall design of the application and doesn't introduce any backwards compatibility issues.To confirm this behaviour, you could check for any v2 configuration handling related to schedules:
#!/bin/bash # Search for any v2 configuration handling related to schedules rg --type go 'v2.*schedule' .If there are any matches, review them to ensure they align with this comment. Additionally, consider updating the user documentation to clearly explain this version-specific behaviour.
wrapper_test.go (1)
1731-1762: Summary of changes and suggestionsThe addition of TestRunUnlock and TestRunInit functions improves the test coverage for the resticWrapper struct, specifically for the 'unlock' and 'init' commands. However, there are opportunities to enhance these tests:
- Align the context command with the command being tested for clarity.
- Remove unused request arguments to avoid confusion.
- Expand the tests to check additional properties of the prepared commands.
- Consider refactoring the two similar test functions into a single table-driven test to reduce code duplication and improve maintainability.
These changes would result in more robust and clear tests, making the codebase easier to maintain and extend in the future.
commands_schedule.go (1)
300-304: Verify logic forIgnoreOnBatteryconditionsThe conditions for handling battery levels might not cover all scenarios. Specifically, if
IgnoreOnBatteryLessThanis zero or negative, the logic may not behave as intended.Please review the logic to ensure all possible values are correctly handled. Consider adding unit tests to validate behaviour when
IgnoreOnBatteryLessThanis zero, negative, or exceeds 100%.commands.go (1)
89-91: Update Command Descriptions ConsistentlyThe
descriptionandlongDescriptionfor theshowcommand have been updated to include both profiles and groups, which enhances clarity and functionality.commands_test.go (6)
110-110: Confirm variable assignments match function signatureVerify that the variables assigned from
getProfileScheduleJobscorrespond to the function's return values.
147-149: Profile group existence assertions are correctThe test correctly verifies that the expected profile groups exist in the configuration.
152-152: Selecting all profiles and groups works as intendedThe test successfully asserts that using
--allselects all profiles and groups.
156-157: Profile selection by name functions correctlyThe test appropriately checks that selecting profiles by name returns the correct profiles.
159-161: Group selection by name functions correctlyThe test accurately verifies that selecting groups by name returns the correct groups.
323-331: Ensure expected output matches the updated priority fieldThe expected output in the test has been updated to reflect the change in the priority field from "background" to "standard". Confirm that this change aligns with the intended behaviour of the application.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #418 +/- ##
==========================================
- Coverage 72.19% 72.17% -0.02%
==========================================
Files 127 128 +1
Lines 13702 13765 +63
==========================================
+ Hits 9891 9934 +43
- Misses 3383 3392 +9
- Partials 428 439 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
docs/content/schedules/commands.md (4)
13-14: Approved with a suggestion for clarityThe updated description effectively clarifies the behaviour of commands before and after version 0.29.0. This information is crucial for users to understand the changes in functionality.
To further enhance clarity, consider adding a brief mention of the new group scheduling feature introduced in v0.29.0. For example:
- These resticprofile commands either operate on the profile selected by `--name`, on the profiles selected by a group (before `v0.29.0`), or on all profiles when the flag `--all` is passed on the command line. + These resticprofile commands either operate on the profile selected by `--name`, on the profiles selected by a group (before `v0.29.0`), on groups (from v0.29.0), or on all profiles when the flag `--all` is passed on the command line.This addition would provide a more complete overview of the command behaviour across versions.
15-19: Excellent addition with a minor grammatical suggestionThe new warning notice is an excellent addition, providing crucial information about the changes in group scheduling behaviour between versions. It clearly explains the differences before and after version 0.29.0, which is vital for users upgrading their systems.
One minor grammatical suggestion:
- Before version `0.29.0`, the `--name` flag on a group was used to select all profiles in the group for scheduling them. It was similar to run the schedule commands on each profile individually. + Before version `0.29.0`, the `--name` flag on a group was used to select all profiles in the group for scheduling them. It was similar to running the schedule commands on each profile individually.This change from "to run" to "to running" improves the grammatical structure of the sentence.
39-41: Valuable information added with a minor grammatical correction neededThis new tip notice provides crucial information about resticprofile's lack of state management for schedules. The recommendation to use the
--allflag before making changes to profiles is excellent advice that can help users avoid potential issues.Please correct the following minor grammatical error:
- resticprofile **does not keep a state** of the schedule and unschedule commands. If you need to do many changes in your profiles (like moving, renaming, deleting etc.) **it's recommended to unschedule everything using the `--all` flag before changing your profiles.** + resticprofile **does not keep a state** of the schedule and unschedule commands. If you need to make many changes to your profiles (like moving, renaming, deleting etc.) **it's recommended to unschedule everything using the `--all` flag before changing your profiles.**This change corrects the phrase "do many changes" to the more idiomatic "make many changes".
🧰 Tools
🪛 LanguageTool
[grammar] ~40-~40: Collocation: With the noun “changes”, you should use the verb “make”.
Context: ...and unschedule commands. If you need to do many changes in your profiles (like mov...(MAKE_A_CHANGE)
47-48: Approved with a minor punctuation suggestionThis update to the unschedule command description is valuable. It clarifies an important limitation of the
--allflag and reinforces the earlier statement about resticprofile not keeping state. This information is crucial for users to understand the behaviour of the unschedule command.Please consider adding a comma for better readability:
- Please note using the `--all` flag won't remove schedules on deleted (or renamed) profiles. **resticprofile does not keep a state of the schedules.** + Please note, using the `--all` flag won't remove schedules on deleted (or renamed) profiles. **resticprofile does not keep a state of the schedules.**This minor change improves the flow of the sentence without altering its meaning.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~47-~47: Possible missing comma found.
Context: ...e selected profile or profiles. Please note using the--allflag won't remove sch...(AI_HYDRA_LEO_MISSING_COMMA)
commands.go (3)
96-103: LGTM: Updated 'schedule' command to support groupsThe changes to the 'schedule' command description and flags are appropriate and consistent with the new functionality to support groups. The new
--allflag is a useful addition.Consider updating the long description to explicitly mention groups as well:
- longDescription: "The \"schedule\" command registers declared schedules of the selected profile (or of all profiles) as scheduled jobs within the scheduling service of the operating system", + longDescription: "The \"schedule\" command registers declared schedules of the selected profile or group (or of all profiles and groups) as scheduled jobs within the scheduling service of the operating system",
118-122: LGTM: Updated 'status' command to support groupsThe changes to the 'status' command long description and flag description are appropriate and consistent with the new functionality to support both profiles and groups.
Consider updating the description to explicitly mention groups as well:
- description: "display the status of scheduled jobs of a profile (or of all profiles)", + description: "display the status of scheduled jobs of a profile or group (or of all profiles and groups)",
Line range hint
200-257: LGTM: Refactored 'showProfileOrGroup' function to support groupsThe changes to the 'showProfileOrGroup' function are well-implemented and consistent with the new functionality to support both profiles and groups. The use of the
config.Schedulableinterface provides a flexible solution for handling both types.For consistency, consider updating the error message on line 234 to match the style of the previous error messages:
- return fmt.Errorf("profile or group '%s' not found", flags.name) + return fmt.Errorf("profile or group '%s': not found", flags.name)config/config.go (1)
356-367: Excellent implementation of the duplicate name check!The new code efficiently detects duplicate profile and group names using a map, which addresses the performance concerns raised in the previous review. This approach has a time complexity of O(n), where n is the total number of profile and group names, making it more scalable for larger configurations.
A minor suggestion for improvement:
Consider formatting the error message for better readability, especially when there are multiple duplicates.You could modify the error message formatting slightly for improved readability:
- clog.Errorf("Duplicate profile and group names found: %s", strings.Join(duplicates, ", ")+". This could lead to some random behaviour.") + clog.Errorf("Duplicate profile and group names found: %s. This could lead to unpredictable behaviour.", strings.Join(duplicates, ", "))This change separates the list of duplicates from the warning message and replaces "random" with "unpredictable" for more precise terminology.
commands_test.go (4)
94-94: Update error assertion methodConsider using
assert.ErrorIsinstead ofassert.EqualErrorfor a more robust error comparison. This aligns with the changes made in other tests for consistency in error handling.
156-157: Consistent testing of profile selectionWhen testing profile selection by name, ensure that the function
selectProfilesAndGroupsreturns only the specified profile and not groups with the same name.
159-161: Consistent testing of group selectionSimilar to profile selection, verify that selecting groups by name returns only the specified group without unintended profiles.
Line range hint
211-212: Address the TODO: Enable loading all schedules without a default profileThe TODO comment indicates an enhancement to allow loading all schedules without requiring a default profile. Implementing this feature would improve usability when no default profile is specified.
Would you like assistance in developing this feature or creating a GitHub issue to track it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- Makefile (1 hunks)
- commands.go (4 hunks)
- commands_test.go (5 hunks)
- config/config.go (2 hunks)
- config/group_test.go (1 hunks)
- config/profile_test.go (1 hunks)
- docs/content/schedules/commands.md (3 hunks)
- wrapper_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- config/group_test.go
- config/profile_test.go
- wrapper_test.go
🧰 Additional context used
🪛 LanguageTool
docs/content/schedules/commands.md
[grammar] ~40-~40: Collocation: With the noun “changes”, you should use the verb “make”.
Context: ...and unschedule commands. If you need to do many changes in your profiles (like mov...(MAKE_A_CHANGE)
[uncategorized] ~47-~47: Possible missing comma found.
Context: ...e selected profile or profiles. Please note using the--allflag won't remove sch...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (12)
docs/content/schedules/commands.md (2)
4-4: LGTM: Version tag updated appropriatelyThe addition of the "v0.29.0" tag is correct and aligns with the new features introduced in this version, particularly the group scheduling functionality.
Line range hint
77-79: Remove empty notice blockThere's an empty notice block at the end of the document that serves no purpose and may confuse readers. As mentioned in a previous review comment, it's best to remove this block if it's not being used.
Please remove the following lines:
{{% notice info %}} {{% /notice %}}If this was intended as a placeholder for future content, it's better to add it back when the content is ready to be included.
commands.go (4)
89-91: LGTM: Updated 'show' command to support groupsThe changes to the 'show' command description and action are appropriate and consistent with the new functionality to support both profiles and groups.
108-113: LGTM: Updated 'unschedule' command to support groupsThe changes to the 'unschedule' command description, long description, and flag description are appropriate and consistent with the new functionality to support both profiles and groups.
127-127: LGTM: Updated 'run-schedule' command description to include groupsThe change to the 'run-schedule' command long description is appropriate and consistent with the new functionality to support both profiles and groups.
252-252: LGTM: Updated 'showSchedules' function callThe change to use
maps.Values()when callingshowSchedulesis appropriate and suggests an update in how schedules are stored or returned.Please verify that the
Schedules()method of theconfig.Schedulableinterface returns a map of schedules. Run the following script to check the method signature:commands_test.go (6)
56-57: Update variable assignments to match function signature changesThe function
getRemovableScheduleJobsnow returns two values(schedules []*config.Schedule, err error), but the code is still unpacking three values. This will result in a compilation error.
110-110: Ensure correct handling of profile and schedulesVerify that the
getProfileScheduleJobsfunction correctly returns bothprofileandschedules. Ensure that theprofilevariable is utilised appropriately in the subsequent assertions.
147-149: Confirm group existence checksEnsure that the
cfg.HasProfileGroup(g)method accurately checks for the existence of groups. This is important for the reliability of tests that depend on group selection.
152-152: Test all profiles and groups selectionThe assertion tests the selection of all profiles and groups with
--allflag. Confirm thatselectProfilesAndGroupscorrectly aggregates profiles and groups without duplicates.
165-165: Review handling of non-existent profiles or groupsWhen selecting a non-existent profile or group,
selectProfilesAndGroupsreturns the provided name. This could lead to issues if the non-existent name is processed further without validation. Consider modifying the function to return an error or an empty list in such cases.
323-323: Verify the change of priority value from 'background' to 'standard'The priority field in the expected output has changed from 'background' to 'standard'. Please ensure this alteration is intentional and consistent with the application's scheduling logic.
Also applies to: 331-331
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
schedule/schedule_test.go (4)
41-43: LGTM: Improved platform checkThe use of
platform.IsWindows()enhances readability and maintainability. This change aligns well with the PR objective of refactoring platform checks.Consider updating the skip message to be more specific:
- t.Skip("crond scheduler is not supported on this platform") + t.Skip("crond scheduler is not supported on Windows")This would provide clearer information in test logs.
51-53: LGTM: Enhanced platform checksThe use of
platform.IsWindows()andplatform.IsDarwin()improves code clarity and maintainability. This change is in line with the PR's goal of refactoring platform checks.Consider updating the skip message to be more specific:
- t.Skip("systemd scheduler is not supported on this platform") + t.Skip("systemd scheduler is not supported on Windows or macOS")This would provide more precise information in test logs.
60-62: LGTM: Improved Darwin checkThe use of
platform.IsDarwin()enhances code readability and maintainability. This change aligns well with the PR objective of refactoring platform checks.Consider updating the skip message to be more specific:
- t.Skip("launchd scheduler is not supported on this platform") + t.Skip("launchd scheduler is only supported on macOS")This would provide clearer information in test logs.
69-71: LGTM: Enhanced Windows checkThe use of
platform.IsWindows()improves code clarity and maintainability. This change is in line with the PR's goal of refactoring platform checks.Consider updating the skip message to be more specific:
- t.Skip("windows scheduler is not supported on this platform") + t.Skip("Windows scheduler is only supported on Windows")This would provide more precise information in test logs.
commands_schedule_test.go (4)
16-47: Configuration string is well-structured, but consider extracting it to a separate file.The YAML configuration string is comprehensive and covers various scheduling scenarios, which is excellent for testing. However, for improved readability and maintainability, consider moving this configuration to a separate YAML file and loading it in the test.
You could create a file named
test_config.yamlin the same directory and load it in the test like this:configData, err := os.ReadFile("test_config.yaml") require.NoError(t, err) cfg, err := config.Load( bytes.NewBuffer(configData), config.FormatYAML, )This approach would make it easier to update the test configuration independently of the test logic.
49-57: Test function structure is solid, but error handling could be improved.The test function is well-structured, using table-driven tests and proper setup with a temporary directory. However, the error handling when loading the configuration could be more robust.
Consider adding more specific assertions for the configuration loading process:
cfg, err := config.Load( bytes.NewBufferString(fmt.Sprintf(scheduleIntegrationTestsConfiguration, crontab)), config.FormatYAML, ) require.NoError(t, err, "Failed to load configuration") require.NotNil(t, cfg, "Loaded configuration is nil") assert.Equal(t, "2", cfg.GetString("version"), "Unexpected configuration version") assert.NotEmpty(t, cfg.GetStringMap("profiles"), "No profiles found in configuration")This approach provides more detailed feedback if the configuration loading fails, making it easier to diagnose issues.
58-75: Test cases are good, but could be more comprehensive.The current test cases cover the main scenarios well. However, there's room for improvement in terms of clarity and coverage.
Consider the following enhancements:
Use a more descriptive name for the missing profile case:
{ name: "missing_profile", err: config.ErrNotFound, },Add more test cases to cover edge cases, such as:
- A profile with an invalid schedule
- A group schedule
- A profile with multiple schedules
Include expected crontab entries in the test cases to verify the correct schedule translation.
Example:
{ name: "profile_with_invalid_schedule", profile: "invalid_schedule_profile", contains: "Error: Invalid schedule", err: ErrInvalidSchedule, }, { name: "group_schedule", profile: "two-profiles", contains: "Group schedule", },These additions would improve the robustness of your tests and catch more potential issues.
77-102: Error handling and assertions are good, with room for enhancement.The use of
require.NoError(),assert.ErrorIs(), andassert.Contains()is appropriate for the different scenarios. Capturing output in a buffer is a good practice for testing CLI applications.Consider the following improvements:
Use
t.Helper()in a separate function for common setup code:func setupTestContext(t *testing.T, name string, cfg *config.Config) commandContext { t.Helper() return commandContext{ Context: Context{ config: cfg, flags: commandLineFlags{ name: name, }, }, } }Consider using
assert.Equal()for more precise output checking, if possible:assert.Equal(t, tc.expectedOutput, output.String(), "Unexpected output")Instead of logging the output unconditionally, consider logging only on test failure:
if t.Failed() { t.Logf("Test case %s failed. Output:\n%s", tc.name, output.String()) }These changes would make your tests more maintainable and provide clearer output when tests fail.
commands.go (4)
96-103: LGTM: Enhanced 'schedule' command with group support and '--all' flagThe updates to the 'schedule' command description and flags are appropriate, reflecting the new capability to handle both profiles and groups. The addition of the '--all' flag is a useful feature for users managing multiple configurations.
Consider a minor improvement to the '--all' flag description for clarity:
- "--all": "add all scheduled jobs of all profiles and groups", + "--all": "add scheduled jobs for all profiles and groups",This slight rewording makes the flag's purpose more explicit.
108-113: LGTM: Updated 'unschedule' command with group support and '--all' flagThe changes to the 'unschedule' command are appropriate and consistent with the updates made to the 'schedule' command. The addition of group support and the '--all' flag provides symmetry and a comprehensive way to manage scheduled jobs.
For consistency with the suggested improvement to the 'schedule' command, consider updating the '--all' flag description:
- flags: map[string]string{"--all": "remove all scheduled jobs of all profiles and groups"}, + flags: map[string]string{"--all": "remove scheduled jobs for all profiles and groups"},This change aligns the wording with the 'schedule' command and improves clarity.
117-122: LGTM: Enhanced 'status' command with group support and '--all' flagThe updates to the 'status' command are appropriate and align well with the changes made to the 'schedule' and 'unschedule' commands. The addition of group support and the '--all' flag provides a comprehensive way to view the status of all scheduled jobs.
For consistency with the suggested improvements to the other commands, consider updating the '--all' flag description:
- flags: map[string]string{"--all": "display the status of all scheduled jobs of all profiles and groups"}, + flags: map[string]string{"--all": "display the status of scheduled jobs for all profiles and groups"},This change aligns the wording with the other commands and improves clarity.
Line range hint
200-257: LGTM with suggestion: Comprehensive update to support profiles and groupsThe changes to the 'showProfileOrGroup' function are well-implemented and consistent with the new functionality to support both profiles and groups. The error handling has been improved, providing clear messages for various scenarios.
However, there's still some duplication in the error handling logic for profiles and groups. To improve maintainability and readability, consider refactoring this function as suggested in a previous review:
Create a helper function to handle loading either a profile or a group:
func loadProfileOrGroup(c *config.Config, name string) (config.Schedulable, error) { if c.HasProfile(name) { profile, cleanup, err := openProfile(c, name) if err != nil { return nil, fmt.Errorf("profile '%s': %w", name, err) } defer cleanup() if profile == nil { return nil, fmt.Errorf("cannot load profile '%s'", name) } return profile, nil } else if c.HasProfileGroup(name) { group, err := c.GetProfileGroup(name) if err != nil { return nil, fmt.Errorf("group '%s': %w", name, err) } if group == nil { return nil, fmt.Errorf("cannot load group '%s'", name) } return group, nil } return nil, fmt.Errorf("profile or group '%s': %w", name, config.ErrNotFound) }Then, simplify the 'showProfileOrGroup' function:
func showProfileOrGroup(output io.Writer, ctx commandContext) error { c := ctx.config flags := ctx.flags // Load global section global, err := c.GetGlobalSection() if err != nil { return fmt.Errorf("cannot load global section: %w", err) } profileOrGroup, err := loadProfileOrGroup(c, flags.name) if err != nil { return err } // ... rest of the function remains the same }This refactoring eliminates duplication and makes the function more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- commands.go (4 hunks)
- commands_schedule.go (1 hunks)
- commands_schedule_test.go (1 hunks)
- commands_test.go (5 hunks)
- config/config.go (2 hunks)
- config/schedule_test.go (3 hunks)
- docs/content/schedules/commands.md (3 hunks)
- schedule/schedule_test.go (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- commands_schedule.go
- config/config.go
- config/schedule_test.go
- docs/content/schedules/commands.md
🧰 Additional context used
🔇 Additional comments (13)
schedule/schedule_test.go (2)
8-8: LGTM: Appropriate package import addedThe addition of the
platformpackage import is consistent with the changes made in the test functions. This change improves code modularity by centralising platform-specific checks.
Line range hint
1-74: Overall assessment: Excellent refactoringThe changes in this file consistently replace
runtime.GOOSchecks withplatform.Is*()functions, which significantly improves code readability and maintainability. This refactoring aligns well with the PR objectives and centralises platform-specific checks.The logic of the tests remains unchanged, ensuring that the existing functionality is preserved. The suggested minor improvements to the skip messages would further enhance the clarity of the test output.
Well done on this clean and effective refactoring!
commands_schedule_test.go (1)
3-14: LGTM: Import statements are appropriate and well-organised.The import statements include all necessary packages for testing, file operations, and assertions. The use of the testify package for assertions is a good practice in Go testing.
commands.go (2)
89-91: LGTM: Updated 'show' command to support groupsThe changes to the 'show' command description and action are appropriate and consistent with the new functionality to support both profiles and groups. The renaming of the action to 'showProfileOrGroup' is a good choice, clearly indicating its expanded purpose.
Line range hint
127-134: LGTM: Updated 'run-schedule' command descriptionThe change to the 'run-schedule' command's long description is appropriate and consistent with the overall updates to support groups. It clearly communicates that the command can now run scheduled jobs for both profiles and groups, which is essential information for users.
commands_test.go (8)
94-95: Error handling for non-existent profiles is correctThe test correctly asserts that
getProfileScheduleJobsreturnsconfig.ErrNotFoundwhen a non-existent profile is requested.
100-102: Assertion for no schedules found is appropriateThe test properly checks that an error is returned when no schedules are found for the profile
'other', and the error message matches the expected output.
Line range hint
110-118: Test accurately retrieves and validates schedules for the default profileThe test successfully retrieves the profile and schedules for
'default'and performs the necessary assertions to verify correctness.
147-149: Validation of profile groups is correctThe loop correctly asserts the existence of all groups defined in the configuration.
152-152: Selection of all profiles and groups functions as intendedThe test asserts that using the
--allflag returns all profiles and groups, which is the expected behaviour.
156-157: Profile selection by name operates correctlyThe test iterates over all profiles and confirms that selecting each by name returns the correct result.
160-161: Group selection by name operates correctlyThe test iterates over all groups and asserts that selecting each by name returns the correct group.
323-331: Verify consistency in schedule priority changesThe expected output in
TestShowScheduleshas been updated to reflect the change in priority from"background"to"standard". Confirm that this modification aligns with the actual implementation and that the priority value"standard"is used correctly throughout the codebase.To ensure consistency, you may review the schedule configurations and related logic to verify that
"standard"is now the default priority.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
config/profile_test.go (2)
1005-1008: Consider usingos.Unsetenvin adeferstatement.While the current implementation using
t.Cleanup()is correct, consider usingdeferstatements for each environment variable. This approach ensures that the variables are unset even if the test panics before reaching thet.Cleanup()call.Here's a suggested improvement:
- t.Cleanup(func() { - _ = os.Unsetenv("RESTIC_ANY1") - _ = os.Unsetenv("RESTIC_ANY2") - }) + defer func() { + _ = os.Unsetenv("RESTIC_ANY1") + _ = os.Unsetenv("RESTIC_ANY2") + }()This change makes the cleanup more robust and aligns with idiomatic Go practices for resource cleanup.
999-1008: Improve error handling for environment variable setup.While the current implementation is functional, it's advisable to handle potential errors when setting environment variables. This ensures that the test fails gracefully if there's an issue with the test setup.
Consider modifying the code as follows:
- require.NoError(t, os.Setenv("RESTIC_ANY1", "xyz")) - require.NoError(t, os.Setenv("RESTIC_ANY2", "xyz")) + err := os.Setenv("RESTIC_ANY1", "xyz") + require.NoError(t, err, "Failed to set RESTIC_ANY1 environment variable") + err = os.Setenv("RESTIC_ANY2", "xyz") + require.NoError(t, err, "Failed to set RESTIC_ANY2 environment variable")This change provides more context in case of a failure, making it easier to diagnose issues related to environment variable setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- config/profile_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
config/profile_test.go (1)
999-1001: LGTM: Proper assertion of schedulable commands.The code correctly verifies that essential commands like 'backup', 'check', 'forget', 'prune', and 'copy' are included in the schedulable commands. This ensures that the core functionality of the scheduling feature is maintained.
commit c1f3836 Author: Fred <[email protected]> Date: Thu Oct 10 23:20:57 2024 +0100 fix missing test cleanup commit 2502fe0 Author: Fred <[email protected]> Date: Thu Oct 10 23:08:59 2024 +0100 add integration tests commit 7a134bf Author: Fred <[email protected]> Date: Thu Oct 10 21:33:44 2024 +0100 nitpicking comments & add test on schedule Kind commit 8bad517 Author: Fred <[email protected]> Date: Wed Oct 9 21:49:52 2024 +0100 remove test files commit 4279f4a Author: Fred <[email protected]> Date: Wed Oct 9 21:49:10 2024 +0100 address PR comments & fix windows build commit 20e83fe Author: Fred <[email protected]> Date: Wed Oct 9 18:17:00 2024 +0100 implement group scheduling commit 78a2a19 Author: Fred <[email protected]> Date: Tue Oct 8 21:45:00 2024 +0100 add support for group schedule
creativeprojects
force-pushed
the
group-schedule
branch
from
c1f3836 to
e629436
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (27)
config/group.go (2)
48-59: LGTM: SchedulableCommands method implemented correctlyThe method returns the expected list of schedulable commands. However, there's room for improvement:
- The comment suggests a future refactoring once the deprecated retention schedule is removed.
- Consider using a constant or variable for the list of commands to improve maintainability.
Would you like me to create a GitHub issue to track the future refactoring task?
Line range hint
1-64: Summary: Group scheduling functionality implemented successfullyThe changes to
config/group.gosuccessfully implement the group scheduling functionality:
- The
SchedulableCommandsmethod provides a list of commands that can be scheduled for a group.- The
Kindmethod identifies this as a Group type for scheduling purposes.These additions, along with the existing
Schedulesmethod, ensure that theGroupstruct fully implements theSchedulableinterface. This aligns well with the PR objective of implementing group scheduling functionality.Consider documenting the
Schedulableinterface and its implementations (Group and Profile) in a central location to aid future developers in understanding and maintaining this scheduling system.commands_schedule_test.go (2)
16-47: LGTM: Well-structured test configuration.The
scheduleIntegrationTestsConfigurationconstant provides a comprehensive YAML configuration for testing various scheduling scenarios. The use of a placeholder for the crontab file path is a good approach for dynamic test setup.Consider adding a brief comment explaining the purpose and structure of this configuration constant. This would enhance readability and maintainability for other developers who might work on these tests in the future.
77-102: LGTM: Well-implemented test execution with proper assertions.The test execution loop is well-structured, using subtests for each case, which is a good practice. The setup of the command context, output capture, and assertions are all implemented correctly.
Consider adding a check for the length of the output when asserting its contents. This could help catch unexpected behaviours where the output is empty or much shorter than expected. For example:
assert.NotEmpty(t, output.String(), "Output should not be empty") assert.Contains(t, output.String(), tc.contains, "Output should contain expected string")This additional assertion would provide more detailed feedback if a test were to fail unexpectedly.
docs/content/schedules/commands.md (3)
13-14: LGTM: Clear explanation of command behaviour across versionsThe updated text provides a clear explanation of how the commands operate on profiles and groups, distinguishing between behaviour before and after v0.29.0. This aligns well with the changes mentioned in the summary.
Consider adding a brief example to illustrate the difference in behaviour, which might help users understand the change more quickly.
39-41: LGTM: Valuable tip about resticprofile's stateless natureThe new tip notice provides crucial information about resticprofile not keeping state of schedule commands. This is important for users to understand, especially when making multiple changes to their profiles.
Consider adding a brief explanation of why this stateless approach was chosen, if there are specific benefits or trade-offs involved.
47-48: LGTM: Important clarification about the --all flagThe added explanation about the --all flag not removing schedules on deleted or renamed profiles is crucial. It reinforces the stateless nature of resticprofile and helps prevent potential user confusion.
Consider adding a brief recommendation on how users should handle schedules for deleted or renamed profiles to ensure a clean state.
docs/content/_index.md (1)
Line range hint
1-37: Consider updating the introductory paragraph.The new group scheduling feature is significant enough to warrant a mention in the introductory paragraph. Consider adding a brief mention of this capability to the list of key features at the beginning of the document.
Here's a suggested addition to the bullet points in the introductory section:
* You can create groups of profiles that will run sequentially + * You can schedule groups of profiles (as of v0.29.0) * You can run [shell commands]({{% relref "/configuration/run_hooks" %}}) before or after running a profile: useful if you need to mount and unmount your backup disk for example🧰 Tools
🪛 Markdownlint
40-40: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
40-40: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
40-40: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
40-40: Expected: asterisk; Actual: underscore
Emphasis style(MD049, emphasis-style)
README.md (3)
43-43: Excellent addition for battery-powered systems.This feature is a valuable addition, particularly for laptop users. It helps prevent unexpected battery drain by not scheduling jobs when the system is running on battery power.
Consider adding a brief note on how users can configure or override this behaviour if needed.
44-44: Valuable new feature for group scheduling.The ability to schedule a group of profiles is a significant enhancement that increases the tool's flexibility. Well done on implementing this feature.
Consider expanding the description to briefly explain the benefits of group scheduling. For example: "This feature allows users to efficiently manage and execute multiple related profiles as a single unit, streamlining the backup process for complex setups."
64-67: Excellent initiative to gather user feedback.Adding a survey section to collect user feedback on important features is a commendable step. It demonstrates a commitment to user-driven development.
To increase visibility and participation, consider moving this section higher up in the README, perhaps just after the feature list. Additionally, you might want to add a brief sentence explaining how the feedback will be used to improve the project.
examples/dev.yaml (1)
137-137: Consider documenting the schedule priority change and its implications.The modification of the 'schedule-priority' from 'standard' to 'background' for the forget operation in the 'self' profile aligns with recent changes in the project's default values. However, it's worth considering the following points:
- This change may affect how the forget operation is scheduled and executed, potentially impacting system resources.
- The implications on the overall backup strategy should be carefully evaluated.
To enhance maintainability and clarity, consider adding a comment explaining the rationale behind this change and its expected impact on the profile's behaviour.
Makefile (1)
76-76: Approved: Version pinning for 'eget' toolThe change from '@latest' to '@v1.3.4' for the 'eget' tool is a good practice. It enhances build reproducibility by ensuring a consistent version across different environments.
Consider updating the project's documentation (e.g., README.md or CONTRIBUTING.md) to mention this specific version requirement for 'eget'. This will help other contributors understand the project's dependencies more clearly.
config/schedule.go (2)
251-254: LGTM: New methods added to Schedulable interfaceThe additions of
SchedulableCommands()andKind()methods enhance the functionality of theSchedulableinterface. These changes appear to improve the flexibility and type awareness of schedulable entities.Consider adding inline documentation for these new methods to clarify their purpose and expected return values. For example:
// SchedulableCommands returns a list of commands that can be scheduled SchedulableCommands() []string // Kind returns the kind of the schedule origin (e.g., "profile" or "group") Kind() string
145-145: LGTM: Simplified String() methodThe
String()method forScheduleConfigOriginhas been simplified, which improves readability and maintainability. This change provides a consistent string representation regardless of the origin type.Consider using the
fmt.Sprintf()function with field names for improved clarity:return fmt.Sprintf("%s@%s", o.Command, o.Name)This makes the code slightly more self-documenting, especially if the struct fields are ever renamed.
config/config.go (1)
356-367: Excellent addition for duplicate name detection.This new code efficiently checks for duplicate profile and group names, which is crucial for preventing conflicts and ensuring predictable behaviour. The implementation is sound, using a map for O(1) lookups.
A minor suggestion for improved clarity:
Consider adding a comment explaining the purpose of this check at the beginning of the block. For example:
+// Check for duplicate profile and group names to prevent conflicts allNames := make(map[string]bool) duplicates := make([]string, 0) for _, name := range append(c.GetProfileNames(), c.GetGroupNames()...) { if allNames[name] { duplicates = append(duplicates, name) } else { allNames[name] = true } } if len(duplicates) > 0 { clog.Errorf("Duplicate profile and group names found: %s. This could lead to unpredictable behaviour.", strings.Join(duplicates, ", ")) }commands.go (9)
89-90: Consistent Capitalisation in DescriptionsConsider capitalising the first letter in the descriptions for consistency. Change "show all the details of the current profile or group" to "Show all the details of the current profile or group".
Apply this diff to improve consistency:
-description: "show all the details of the current profile or group", +description: "Show all the details of the current profile or group",
96-97: Enhance Clarity in Command DescriptionThe description can be made clearer by rephrasing it to reflect the action more directly.
Consider rewording as:
-description: "schedule jobs from a profile or group (or of all profiles and groups)", +description: "Schedule jobs from a profile or group (or from all profiles and groups)",
103-103: Improve Flag Description for ClarityThe description for the
--allflag can be clearer. Specify that it schedules all jobs from all profiles and groups.Apply this diff for better clarity:
-"--all": "add all scheduled jobs of all profiles and groups", +"--all": "Add all scheduled jobs from all profiles and groups",
108-109: Refine Command Description for ConsistencyFor consistency and readability, rephrase the description to match the style of other commands.
Consider this update:
-description: "remove scheduled jobs of a profile or group (or of all profiles and groups)", +description: "Remove scheduled jobs from a profile or group (or from all profiles and groups)",Also, in the
longDescription, consider rewording to improve clarity:-The command removes jobs for schedules declared in the selected profile or group (or of all profiles and groups) +It removes jobs for schedules declared in the selected profile or group (or from all profiles and groups).
113-113: Consistent Capitalisation in Flag DescriptionsEnsure that flag descriptions start with a capital letter for consistency across the command definitions.
Apply this diff:
-flags: map[string]string{"--all": "remove all scheduled jobs of all profiles and groups"}, +flags: map[string]string{"--all": "Remove all scheduled jobs from all profiles and groups"},
117-118: Enhance Clarity in Command DescriptionRephrase the description to improve clarity and readability.
Consider updating as follows:
-description: "display the status of scheduled jobs of a profile or group (or of all profiles and groups)", +description: "Display the status of scheduled jobs from a profile or group (or from all profiles and groups)",Similarly, in the
longDescription, you can enhance clarity:-"status" command prints all declared schedules of the selected profile or group (or of all profiles and groups) +"status" command prints all declared schedules of the selected profile or group (or from all profiles and groups)
122-122: Improve Flag Description for ClarityFor the
--allflag, adjust the description for better understanding.Apply this diff:
-flags: map[string]string{"--all": "display the status of all scheduled jobs of all profiles and groups"}, +flags: map[string]string{"--all": "Display the status of all scheduled jobs from all profiles and groups"},
127-127: Clarify Parameter Formatting in DescriptionIn the
longDescriptionforrun-schedule, wrap the parameter format in backticks to clearly indicate command-line syntax.Apply this diff:
-The name in parameter is <command>@<profile-or-group-name>. +The name in parameter is `<command>@<profile-or-group-name>`.
229-230: Correct Error Message for ConsistencyThe error message "cannot load group '%s'" can be more informative by including the underlying error.
Consider updating the error message:
-if group == nil { - return fmt.Errorf("cannot load group '%s'", flags.name) +if group == nil { + return fmt.Errorf("group '%s' not found", flags.name)config/schedule_test.go (1)
402-491: Consider clarifying test case with an empty profile nameIn the
TestScheduleVersion2function, the test case wherenameis an empty string may lead to ambiguity.{ name: "", err: ErrNotFound, schedules: map[string]*Schedule{}, },To improve clarity, consider using a non-existent profile name such as
"nonexistent":{ - name: "", + name: "nonexistent", err: ErrNotFound, schedules: map[string]*Schedule{}, },config/profile.go (1)
972-974: Consistent Receiver Naming: Rename Receiver fromotopFor consistency with other methods of the
Profilestruct, consider renaming the receiver fromotop.Apply this diff to rename the receiver:
- func (o *Profile) Kind() string { + func (p *Profile) Kind() string { return constants.SchedulableKindProfile }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
- Makefile (1 hunks)
- README.md (2 hunks)
- commands.go (4 hunks)
- commands_schedule.go (1 hunks)
- commands_schedule_test.go (1 hunks)
- commands_test.go (5 hunks)
- config/config.go (2 hunks)
- config/group.go (3 hunks)
- config/group_test.go (1 hunks)
- config/profile.go (2 hunks)
- config/profile_test.go (1 hunks)
- config/schedule.go (4 hunks)
- config/schedule_test.go (3 hunks)
- constants/schedule.go (1 hunks)
- deprecation.go (1 hunks)
- docs/content/_index.md (1 hunks)
- docs/content/schedules/commands.md (3 hunks)
- examples/dev.yaml (1 hunks)
- examples/dynamic.yaml (1 hunks)
- run_profile.go (1 hunks)
- schedule/schedule_test.go (5 hunks)
- wrapper_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- constants/schedule.go
🧰 Additional context used
🔇 Additional comments (27)
config/group_test.go (2)
3-7: Imports look goodThe imports are appropriate for a test file. Using the
assertpackage fromtestifyis a good practice for writing clear and expressive tests.
1-13: Overall assessment: Good start, with room for improvementThe new test file introduces a basic test for the
SchedulableCommandsmethod, which is a positive addition to the test suite. The test covers the main functionality, but there's potential for enhancement to make it more robust and informative.Consider implementing the suggested improvements to increase test coverage and clarity. This will help ensure the reliability and maintainability of the
Groupstruct and its methods.examples/dynamic.yaml (2)
3-6: Excellent enhancement to the$databasesvariable initialization!This change improves the robustness of the configuration:
- It initialises
$databasesas an empty list by default.- It only populates
$databasesfrom the environment variable if it's defined.This approach gracefully handles scenarios where the
.Env.databasesenvironment variable is not set, preventing potential errors and making the configuration more flexible.
Line range hint
1-38: The changes are fully compatible with the existing configuration.The updated
$databasesinitialization works seamlessly with the rest of the configuration:
- If
.Env.databasesis set, the configuration behaves as before.- If
.Env.databasesis not set, thegroupsandprofilessections using$databaseswill effectively be skipped, as they'll iterate over an empty list.This change enhances the flexibility of the configuration without requiring any modifications to the existing structure.
schedule/schedule_test.go (5)
8-8: Import of platform package looks goodThe addition of the
platformpackage import is appropriate for the subsequent changes in the file. It replaces the use ofruntime.GOOSwith more specific platform check functions, which can improve code readability and maintainability.
41-41: Verify the removal of Darwin check in TestCrondInitThe condition has been simplified to only check for Windows, but the previous version also skipped the test on Darwin (macOS). Please confirm if this change is intentional, as it might lead to unexpected test execution on macOS systems.
Consider updating the condition to:
if platform.IsWindows() || platform.IsDarwin() { t.Skip("crond scheduler is not supported on this platform") }
51-51: TestSystemdInit platform check looks goodThe update to use
platform.IsWindows()andplatform.IsDarwin()is a good improvement. It maintains the same logic as before whilst using the new platform-specific functions, which enhances code readability and consistency.
60-60: TestLaunchdInit platform check is correctThe update to use
!platform.IsDarwin()is a good improvement. It maintains the same logic as before whilst using the new platform-specific function, which enhances code readability and consistency.
69-69: TestWindowsInit platform check is appropriateThe update to use
!platform.IsWindows()is a good improvement. It maintains the same logic as before whilst using the new platform-specific function, which enhances code readability and consistency.config/group.go (2)
3-6: LGTM: Import statement updated appropriatelyThe addition of the constants package import is consistent with its usage in the new methods.
61-63: LGTM: Kind method implemented correctlyThe method returns the appropriate constant value for the Group type, fulfilling its role in the Schedulable interface.
commands_schedule_test.go (1)
1-14: LGTM: Package declaration and imports are appropriate.The package declaration and imports are well-structured and relevant for a test file in the main package. The use of the
testifypackage for assertions is a good practice for writing clear and concise tests.docs/content/schedules/commands.md (2)
4-4: LGTM: Version tag updated appropriatelyThe addition of the "v0.29.0" tag is consistent with the introduction of new features, particularly group scheduling. This update helps users identify which version of the software this documentation applies to.
15-19: LGTM: Informative warning about changes in group schedulingThe new warning notice effectively communicates the changes in group scheduling behaviour between versions. This is crucial information for users upgrading from older versions and helps prevent confusion about the new functionality.
docs/content/_index.md (1)
37-37: New feature documentation looks good.The new line accurately describes the addition of group scheduling functionality in version 0.29.0. It's consistent with the format of other feature announcements in the list and provides clear information about the version and configuration requirements.
run_profile.go (1)
132-132: Approve the function name change and suggest verification.The change from
displayProfileDeprecationNoticestodisplayDeprecationNoticesappears to be part of a broader effort to support both profiles and groups. This aligns well with the PR objectives.To ensure the change is implemented correctly, please run the following verification script:
config/schedule.go (2)
54-54: LGTM: Consistent update of default priorityThe change in the
scheduleBaseConfigDefaultsvariable is consistent with the modification in theScheduleBaseConfigstruct, ensuring that the default priority is uniformly set to "standard".
40-40:⚠️ Potential issueDefault priority changed from "background" to "standard"
This change may affect the behaviour of scheduled tasks, potentially allocating more system resources to them by default. Please ensure this aligns with the intended system behaviour and consider documenting this change for users who may be relying on the previous default.
To verify the impact of this change, please run the following script:
✅ Verification successful
Default priority change verified
The default priority in
config/schedule.gohas been successfully updated from "background" to "standard". All references to "background" in other parts of the codebase are intentional and maintain necessary flexibility for specific use cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded "background" priority settings that might need updating rg -i 'priority.*background' --type goLength of output: 1697
config/config.go (1)
535-535: Improved error handling consistency.Excellent change. Replacing the formatted error message with the standard
ErrNotFounderror improves consistency in error handling throughout the codebase. This modification allows for easier error checking and handling by the callers of this method.commands.go (1)
247-247:⚠️ Potential issueEnsure Error Is Not Nil Before Logging
When logging the error message, ensure that
erris not nil to prevent a potential nil pointer dereference.Apply this diff:
-if err != nil { clog.Errorf("cannot show profile or group '%s': %s", flags.name, err.Error()) +if err != nil { + clog.Errorf("cannot show profile or group '%s': %s", flags.name, err.Error()) }Likely invalid or redundant comment.
config/schedule_test.go (4)
336-338: Assertion correctly checks for panic with expected error messageThe test
TestScheduleForProfileEnforcesOriginappropriately usesassert.PanicsWithErrorto ensure that the functionnewScheduleForProfilepanics with the expected error message when used incorrectly.
349-349: Default priority updated to 'standard'The test
TestScheduleBuiltinDefaultsnow correctly asserts that the defaultPriorityis"standard", reflecting the updated default behaviour of the scheduling system.
394-400: Tests verify theKind()method forProfileandGroupThe new
TestScheduleKindfunction accurately tests theKind()method for bothProfileandGroup, ensuring they return the expected values"profile"and"group"respectively.
450-455: Unexported fields in struct comparison may cause issuesWhen asserting equality between
ScheduleConfigstructs:assert.Equal(t, tc.schedules, schedules)Unexported fields like
normalizedare not accessible outside the package and may not be compared as intended. This could lead to tests passing or failing unexpectedly.To ensure accurate comparisons, consider one of the following options:
- Use
assert.EqualValuesinstead ofassert.Equal, which usesreflect.DeepEqualand can compare unexported fields.- Expose the necessary fields for testing purposes.
- Compare only the exported fields relevant to the test assertions.
Would you like assistance in refactoring the tests for accurate comparison?
config/profile_test.go (2)
999-1001: Previous comment is still valid regarding hardcoded command list.
1005-1008: LGTM!The use of
t.Cleanupto unset environment variables is appropriate and ensures the environment variables are properly cleaned up after the test.config/profile.go (1)
311-311: Previous Comment Still Applicable: Updateschedule-priorityUsageThe prior review comment about changing the default value of
schedule-priorityfrom "background" to "standard" remains valid. Please ensure all configuration files are updated to reflect this change to prevent potential scheduling issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
.github/workflows/doc.yml (3)
42-42: Approve the Hugo version update with a minor suggestion.The change from using the 'latest' Hugo version to a specific version '0.135.0' is a good practice. It ensures consistency and reproducibility in the documentation build process across different environments and over time.
However, to future-proof this configuration, consider using a version range instead of a fixed version. This allows for minor updates and patches without manual intervention.
Consider updating the Hugo version specification as follows:
- hugo-version: '0.135.0' + hugo-version: '~0.135.0'This change will allow for minor version updates within the 0.135.x range, ensuring you receive bug fixes and non-breaking changes automatically.
Line range hint
3-11: Consider optimising the workflow trigger conditions.The current configuration triggers the workflow on any push to the master branch that affects the 'docs/**' path. To prevent unnecessary runs, you might want to exclude certain file types within the docs directory that don't affect the build (e.g., .md files).
Consider updating the 'on' section as follows:
on: push: branches: - master paths: - "docs/**" - "!docs/**/*.md" pull_request: types: [opened, synchronize, reopened] paths: - 'docs/**' - '!docs/**/*.md'This change will prevent the workflow from running when only Markdown files are modified, which typically don't affect the build process.
Line range hint
21-23: Consider using a more flexible Go version specification.The current Go version specification (~1.23) is quite specific and might require frequent updates as new Go versions are released.
Consider using a more flexible version specification:
- go-version: ~1.23 + go-version: ^1.23This change will allow for minor and patch version updates, reducing the need for manual intervention while still ensuring compatibility.
.github/workflows/release-doc.yml (2)
36-36: Approval: Fixed Hugo version for consistencyThe change from 'latest' to a specific version ('0.135.0') is a good practice. It ensures consistency and reproducibility of builds across different environments and times.
Consider implementing a centralised version management system for tools like Hugo across all workflow files. This could involve using GitHub Actions variables or a separate configuration file to store version numbers, making it easier to update versions consistently across all workflows in the future.
Line range hint
1-72: Suggestions for workflow optimisationWhile the workflow is well-structured, consider the following minor improvements:
- Remove the commented-out condition
# if: github.ref == 'refs/heads/main'in the "Deploy to GitHub Pages" step if it's no longer needed.- Implement caching for dependencies to speed up subsequent workflow runs. For example, you could cache Go modules and Hugo resources.
Here's an example of how you might implement caching for Go modules:
- uses: actions/cache@v3 with: path: | ~/.cache/go-build ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} restore-keys: | ${{ runner.os }}-go-And for Hugo resources:
- uses: actions/cache@v3 with: path: /tmp/hugo_cache key: ${{ runner.os }}-hugomod-${{ hashFiles('**/go.sum') }} restore-keys: | ${{ runner.os }}-hugomod-These caching steps should be placed before the corresponding setup steps in the workflow.
crond/crontab.go (1)
Line range hint
255-273: Excellent enhancement to support quoted config file namesThe changes effectively implement support for quoted config file names in crontab entries. This improvement enhances the flexibility of the
deleteLinefunction, allowing it to match a wider range of crontab entry formats.A minor suggestion for improved readability:
Consider modifying the regex pattern slightly for better readability:
-search := fmt.Sprintf(`(?m)^[^#][^\n]+resticprofile[^\n]+--config ["]?%s["]? (%s|%s)\n`, +search := fmt.Sprintf(`(?m)^[^#][^\n]+resticprofile[^\n]+--config ["']?%s["']? (%s|%s)\n`,This change allows for both single and double quotes, making the pattern more flexible and easier to understand at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- .github/workflows/doc.yml (1 hunks)
- .github/workflows/release-doc.yml (1 hunks)
- config/group_test.go (1 hunks)
- crond/crontab.go (2 hunks)
- crond/crontab_test.go (1 hunks)
- deprecation.go (1 hunks)
- run_profile.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/group_test.go
- deprecation.go
- run_profile.go
🧰 Additional context used
🔇 Additional comments (2)
.github/workflows/doc.yml (1)
Line range hint
65-72: Review the 'continue-on-error' directive in the deployment step.The 'Publish to pages.dev' step uses 'continue-on-error: true', which allows the workflow to succeed even if the deployment fails. While this is useful for pull requests from forks, it might hide deployment issues for pushes to the main repository.
Consider implementing a conditional step that only runs for pull requests from forks. This would allow you to fail the workflow on deployment errors for pushes to the main repository while still allowing PR checks to pass for fork contributions.
Here's a script to verify if this change is necessary:
If this script returns any results, it indicates that there have been recent failures in the master branch, which might have been hidden by the 'continue-on-error' directive.
crond/crontab_test.go (1)
75-78: Excellent addition of test cases for quoted configuration pathsThe new test cases enhance the coverage of the
deleteLinefunction by including scenarios with quoted configuration file paths. This is a valuable addition that ensures the function correctly handles various formats of crontab entries.These changes align well with the modifications made to the
deleteLinefunction incrond/crontab.go, where the regex pattern was updated to accommodate additional formats for crontab entries.
Implementation of group scheduling