Global json schema (auto versioning) #412
Conversation
WalkthroughThe pull request introduces modifications 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: 7
🧹 Outside diff range and nitpick comments (18)
contrib/templates/nested.gomd (1)
28-30: Excellent addition of dynamic web base URL configuration.This change enhances flexibility by allowing the web base URL to be set via an environment variable. It maintains backwards compatibility whilst providing a way to override the default value, which aligns well with the PR objectives.
For improved clarity, consider adding a comment explaining the purpose of this block:
{{- $webBaseUrl := "https://creativeprojects.github.io/resticprofile" -}} +{{- /* Allow overriding the web base URL via environment variable */ -}} {{- if .Env.WEB_BASE_URL -}} {{- $webBaseUrl = .Env.WEB_BASE_URL -}} {{- end -}}contrib/templates/json-schema.gomd (1)
27-29: Excellent addition for dynamic configurationThis change enhances flexibility by allowing the base URL to be dynamically set via an environment variable. It's a good step towards improving schema management and preparing for SchemaStore integration.
Consider adding a simple URL validation check for the
WEB_BASE_URLvalue to ensure it's a valid URL format. This could prevent potential issues if the environment variable is set incorrectly.contrib/templates/profile.gomd (1)
30-32: Excellent addition of environment variable support.The introduction of the
WEB_BASE_URLenvironment variable check enhances the template's flexibility, allowing for easy customisation of the base URL across different environments.Consider adding a comment explaining the purpose of
WEB_BASE_URLand its expected format. This would improve maintainability and help future developers understand the configuration options. For example:{{- /* WEB_BASE_URL: Optional. Set to override the default base URL for web resources. */ -}} {{- $webBaseUrl := "https://creativeprojects.github.io/resticprofile" -}} {{- if .Env.WEB_BASE_URL -}} {{- $webBaseUrl = .Env.WEB_BASE_URL -}} {{- end -}}docs/content/configuration/jsonschema/index.md (5)
11-16: Excellent update to the Schema URL section.The changes effectively simplify the schema usage by providing a single, dynamic URL. The additional information about schema detection is valuable for users. Well done!
However, consider adding a brief explanation of what "full support for JSON Schema Draft 7" means, as some users might not be familiar with this requirement.
🧰 Tools
🪛 Markdownlint
13-13: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
Line range hint
33-42: Good updates to the vscode usage examples.The YAML and JSON examples have been correctly updated to use the new schema URL, which is consistent with the changes in the previous section.
However, the TOML example still uses the versioned schema URL. Consider adding a brief explanation for why the TOML example differs, perhaps referencing the limited support mentioned earlier in the document.
57-58: Helpful addition of YAML extension information.The mention of the 'redhat.vscode-yaml' extension is valuable for users working with YAML files. This is consistent with the notice about required extensions for YAML and TOML validation.
Consider formatting this information similarly to the notice above for consistency, perhaps using the same notice style.
60-87: Excellent addition of the "Versioned URLs" section.This new section provides comprehensive information about the schema versioning system, clearly explaining the differences between universal and version-specific schemas. The structure and content are well-organised and informative.
Consider adding a brief example of when a user might choose to use a versioned URL over the dynamic one, to help users understand the practical implications of this choice.
🧰 Tools
🪛 LanguageTool
[typographical] ~63-~63: If the word ‘Which’ starts a question, add a question mark (“?”) at the end of the sentence.
Context: ...ed depends on config and restic version. You may use the URLs directly if you ne...(WHAT_NOUNPHRASE_QUESTION_MARK)
[uncategorized] ~71-~71: Possible missing comma found.
Context: ...lags and commands of all known restic versions and may allow to set flags that are no...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~72-~72: Did you mean “setting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... known restic versions and may allow to set flags that are not supported by a parti...(ALLOW_TO)
71-72: Minor grammatical improvement suggestion.Consider revising the sentence structure in these lines for improved clarity:
- These universal schemas contain all flags and commands of all known *restic* versions and - may allow to set flags that are not supported by a particular *restic* version. + These universal schemas contain all flags and commands of all known *restic* versions and + may allow setting flags that are not supported by a particular *restic* version.This change improves the grammatical structure without altering the meaning.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~71-~71: Possible missing comma found.
Context: ...lags and commands of all known restic versions and may allow to set flags that are no...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~72-~72: Did you mean “setting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... known restic versions and may allow to set flags that are not supported by a parti...(ALLOW_TO)
commands_generate.go (1)
77-80: Approve changes with a minor suggestion for clarityThe modification to dynamically set the template name based on the provided arguments is a good improvement. It enhances the flexibility of the function without altering its core logic.
To further improve clarity, consider adding a brief comment explaining the purpose of this change. For example:
name := "config-reference" +// Use the base name of the first argument as the template name if provided if len(args) > 0 { name = filepath.Base(args[0]) }This comment would help future developers understand the intention behind this dynamic naming approach.
config/checkdoc/main.go (1)
25-26: Approve regex change with a minor suggestionThe modification to the
urlPatternregular expression enhances flexibility by accommodating both versioned and non-versioned configuration file names. This change aligns well with the updatedreplaceURLconstant.Consider simplifying the regex slightly by using the
?quantifier directly on the group:-urlPattern = regexp.MustCompile(`{{< [^>}]+config(-\d)?\.json"[^>}]+ >}}`) +urlPattern = regexp.MustCompile(`{{< [^>}]+config(-\d+)?\.json"[^>}]+ >}}`)This change allows for multi-digit version numbers while maintaining the optional nature of the versioning.
config/global.go (1)
21-21: Excellent enhancement to the ResticVersion field.The addition of metadata tags to the
ResticVersionfield is a positive change. It improves configurability and adds validation for the version string. The description tag provides helpful context for users.A minor suggestion: Consider adjusting the regex pattern to allow for pre-release versions (e.g., 2.0.0-beta.1). You could modify it to:
pattern:"^(|[0-9]+\\.[0-9]+(\\.[0-9]+)?(-[0-9A-Za-z-]+(\\.[0-9A-Za-z-]+)*)?)$"This pattern would accommodate semantic versioning more comprehensively.
config/jsonschema/model_test.go (1)
224-228: Good addition of tests forAdditionalPropertiesThe new assertions for the
AdditionalPropertiesfield improve the test coverage and align with the JSON Schema specification. They effectively test the default value, error handling for invalid types, and acceptance of valid types.A minor suggestion to enhance the tests further:
Consider adding an assertion to check that a boolean
truevalue is also accepted forAdditionalProperties, as this is a valid option according to the JSON Schema specification.assert.Equal(t, false, obj.AdditionalProperties) obj.AdditionalProperties = "-" assert.ErrorContains(t, obj.verify(), `additionalProperties must be nil, boolean or SchemaType`) +obj.AdditionalProperties = true +assert.NoError(t, obj.verify()) obj.AdditionalProperties = newSchemaString() assert.NoError(t, obj.verify())This addition would ensure that all valid types for
additionalPropertiesare tested.docs/content/configuration/getting_started/index.md (2)
Line range hint
257-270: LGTM: Schema reference updated and schedule option added.The changes are consistent with the PR objectives:
- The JSON schema reference has been updated to 'config.json'.
- The new 'schedule' option has been added to the backup configuration, enhancing the documentation.
Consider adding a brief comment explaining the purpose of the 'schedule' option, e.g.:
schedule: "12:30" # Run the backup daily at 12:30This would provide immediate context for users reading the configuration example.
Line range hint
293-309: LGTM: Schema reference updated and schedule option added consistently.The changes in the JSON configuration example are consistent with the YAML example and align with the PR objectives:
- The JSON schema reference has been updated to 'config.json'.
- The 'schedule' option has been added to the backup configuration.
For consistency with the suggested improvement in the YAML example, consider adding a comment explaining the 'schedule' option:
"schedule": "12:30", // Run the backup daily at 12:30This would provide immediate context for users reading the JSON configuration example.
Makefile (1)
247-248: LGTM: JSON schema generation command addedThe new command for generating the JSON schema configuration file is well-structured and utilises the project's binary effectively. It's a good addition that aligns with the PR objectives of enhancing JSON schema generation.
However, for improved clarity and maintainability, consider breaking the long command into multiple lines using Makefile's line continuation character (). For example:
$(abspath $(BINARY)) generate \ --config-reference $(CONTRIB_DIR)/templates/config-schema.gojson \ > $(JSONSCHEMA_DIR)/config.jsonThis format enhances readability, especially as the command might grow more complex in the future.
commands_test.go (1)
238-250: LGTM: New test case added for config reference schema generation.The new test case for
--config-reference config-schema.gojsonis well-structured and covers important aspects of the new functionality. It appropriately checks for the presence of schema-related strings and verifies that the output is valid JSON.However, to make the test more robust, consider adding specific checks on the decoded JSON object. For example:
assert.Equal(t, "https://json-schema.org/draft/2020-12/schema", content["$schema"]) assert.Contains(t, content, "properties") assert.Contains(t, content["properties"], "version")This would provide more confidence in the structure of the generated schema.
config/jsonschema/schema_test.go (1)
Line range hint
391-451: Refined and expanded property type assertionsThe improvements to the
Propertiestest case are commendable. The expanded assertions cover a wider range of property types, ensuring they are correctly represented in the generated schema. The use of subtests for each property type enhances the organisation and readability of the test case.For consistency, consider using the
t.Run()subtest approach for the "multiple-str" property as well, similar to how it's done for the other properties.Here's a suggested modification for the "multiple-str" assertion:
- t.Run("multiple-str", func(t *testing.T) { - require.IsType(t, &schemaTypeList{}, schema.Properties["multiple-str"]) - if tl, ok := schema.Properties["multiple-str"].(*schemaTypeList); ok { - assert.IsType(t, &schemaString{}, tl.AnyOf[0]) - require.IsType(t, &schemaArray{}, tl.AnyOf[1]) - assert.Same(t, tl.AnyOf[0], tl.AnyOf[1].(*schemaArray).Items) - } - }) + t.Run("multiple-str", func(t *testing.T) { + require.IsType(t, &schemaTypeList{}, schema.Properties["multiple-str"]) + tl, ok := schema.Properties["multiple-str"].(*schemaTypeList) + require.True(t, ok) + assert.IsType(t, &schemaString{}, tl.AnyOf[0]) + require.IsType(t, &schemaArray{}, tl.AnyOf[1]) + assert.Same(t, tl.AnyOf[0], tl.AnyOf[1].(*schemaArray).Items) + })This change aligns the "multiple-str" test with the structure of the other property tests, improving overall consistency.
contrib/templates/config-schema.gojson (1)
126-170: Consider simplifying theschemaWithResticVersiontemplateThe
schemaWithResticVersiontemplate is complex and might be simplified for better readability. Breaking down nested properties or adding comments within the template could enhance understanding for future maintainers.[maintainability]
You could, for instance, refactor nested objects or provide inline comments explaining each section's purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
- Makefile (2 hunks)
- commands_generate.go (1 hunks)
- commands_test.go (2 hunks)
- config/checkdoc/main.go (1 hunks)
- config/global.go (1 hunks)
- config/jsonschema/model.go (4 hunks)
- config/jsonschema/model_test.go (1 hunks)
- config/jsonschema/schema.go (1 hunks)
- config/jsonschema/schema_test.go (2 hunks)
- config/mocks/NamedPropertySet.go (1 hunks)
- config/mocks/ProfileInfo.go (1 hunks)
- config/mocks/PropertyInfo.go (1 hunks)
- config/mocks/SectionInfo.go (1 hunks)
- contrib/templates/config-schema.gojson (1 hunks)
- contrib/templates/groups.gomd (1 hunks)
- contrib/templates/json-schema.gomd (1 hunks)
- contrib/templates/nested.gomd (1 hunks)
- contrib/templates/profile.gomd (1 hunks)
- docs/content/configuration/getting_started/index.md (4 hunks)
- docs/content/configuration/jsonschema/index.md (4 hunks)
- monitor/mocks/OutputAnalysis.go (1 hunks)
- schedule/mocks/Handler.go (1 hunks)
✅ Files skipped from review due to trivial changes (6)
- config/mocks/NamedPropertySet.go
- config/mocks/ProfileInfo.go
- config/mocks/PropertyInfo.go
- config/mocks/SectionInfo.go
- monitor/mocks/OutputAnalysis.go
- schedule/mocks/Handler.go
🧰 Additional context used
🪛 Markdownlint
docs/content/configuration/jsonschema/index.md
13-13: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
🪛 LanguageTool
docs/content/configuration/jsonschema/index.md
[typographical] ~63-~63: If the word ‘Which’ starts a question, add a question mark (“?”) at the end of the sentence.
Context: ...ed depends on config and restic version. You may use the URLs directly if you ne...(WHAT_NOUNPHRASE_QUESTION_MARK)
[uncategorized] ~71-~71: Possible missing comma found.
Context: ...lags and commands of all known restic versions and may allow to set flags that are no...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~72-~72: Did you mean “setting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... known restic versions and may allow to set flags that are not supported by a parti...(ALLOW_TO)
🔇 Additional comments (12)
contrib/templates/groups.gomd (1)
27-29: Excellent addition of configurable web base URLThis change enhances the flexibility of the template by allowing the web base URL to be configured externally via the
WEB_BASE_URLenvironment variable. It maintains backwards compatibility by retaining the default value when the environment variable is not set. This modification is particularly useful for scenarios where the documentation needs to be hosted at different locations.config/checkdoc/main.go (1)
21-21: Verify the intentionality of URL pattern changeThe URL pattern for configuration files has been modified by removing the hyphen before
$1. This change could affect how configuration files are referenced or accessed.Please confirm that this modification is intentional and aligns with the new naming convention for configuration files. Additionally, ensure that all references to this URL pattern throughout the codebase have been updated accordingly.
To verify the impact, you can run the following script:
✅ Verification successful
URL pattern change is verified
The removal of the hyphen before
$1in thereplaceURLhas been confirmed. No remaining references to the old URL patternconfig-$1.jsonwere found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old URL pattern # Test: Search for the old URL pattern with hyphen rg --type go 'config-\$1\.json' # Test: Search for the new URL pattern without hyphen rg --type go 'config\$1\.json'Length of output: 171
docs/content/configuration/getting_started/index.md (1)
122-122: LGTM: JSON schema reference updated consistently.The JSON configuration example has been updated to use 'config.json', maintaining consistency with the YAML example and supporting the PR objectives.
Makefile (1)
38-38: LGTM: New directory variable addedThe addition of the
CONTRIB_DIRvariable is a good practice. It centralises the location for contribution-related files, making it easier to maintain and update in the future.commands_test.go (1)
5-5: LGTM: New import statement added correctly.The addition of the
encoding/jsonpackage to the import list is appropriate for the new test case involving JSON decoding.config/jsonschema/schema_test.go (3)
360-362: Improved assertion clarity for AdditionalPropertiesThe change from
assert.Trueandassert.Falsetoassert.Equalenhances the readability and precision of the test. This modification makes the expected behaviour more explicit, which is a good practice in unit testing.
377-377: Added assertion for TypedAdditionalProperty behaviourExcellent addition of a new assertion to verify that
AdditionalPropertiesis set to false when a typed additional property is present. This test case ensures that the schema generation correctly handles typed additional properties, which is crucial for maintaining the integrity and correctness of the JSON schema.
Line range hint
381-389: Added test cases for Title and Description propertiesWell done on adding new test cases for the
TitleandDescriptionproperties. These additions enhance the test coverage by ensuring that metadata is correctly transferred from the property set to the generated schema. This improvement helps maintain the accuracy and completeness of the schema generation process.contrib/templates/config-schema.gojson (3)
145-145: Verify correct escaping in regex pattern for version matchingIn the regex pattern used for matching the
restic-version, the dot character is escaped usingreplace "." "\\.". Ensure that this escaping functions as intended in the generated JSON schema. Incorrect escaping might lead to unintended version matching behavior.Consider generating the schema and inspecting the
patternfield to confirm that the dot is properly escaped. The expected pattern should resemble\d+\.\d+\.*.
147-147: Review theminLengthconstraint forrestic-versionThe
minLengthproperty is set to3for therestic-versionstring. Given that version strings could vary in length (e.g.,"0.9","1.10","22.3"), verify that this minimum length does not inadvertently exclude valid version numbers.Consider whether a
patternconstraint alone is sufficient to validate the version format, and if so, you might remove theminLengthproperty.- "minLength": 3
75-91:⚠️ Potential issueValidate the logic for detecting absence of
restic-versionThe definition of
"no-restic-version"is checking for the absence of"restic-version"under"global". Ensure that this logic correctly handles cases where"global"is not defined at all, which could lead to unexpected behavior.Consider adjusting the schema to handle cases where
"global"is missing:{ "not": { "type": "object", "properties": { "global": { + "type": "object", "properties": { "restic-version": {} }, "required": [ "restic-version" ] } }, + "required": [ + "global" + ] } }This ensures that the absence of
"global"and"restic-version"is properly detected.config/jsonschema/model.go (1)
301-309: Ensure theverifymethod robustly handles allAdditionalPropertiestypesThe updated
verifymethod includes a type switch forAdditionalProperties, handlingnil,bool, andSchemaType. Confirm that this logic correctly validates these types and that any invalid types are appropriately flagged with an error. This check is crucial to maintain the integrity of the schema validation process.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #412 +/- ##
==========================================
- Coverage 72.33% 72.32% -0.01%
==========================================
Files 127 127
Lines 12955 12977 +22
==========================================
+ Hits 9370 9385 +15
- Misses 3160 3164 +4
- Partials 425 428 +3
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: 5
🧹 Outside diff range and nitpick comments (1)
commands_generate.go (1)
Line range hint
148-159: Consider handling errors from deferredfile.Close()In the
generateFileFromTemplatefunction, the deferredfile.Close()call does not check for errors that might occur when closing the file. According to best practices, it's advisable to handle errors from theClosemethod to ensure all I/O operations are successful.You could modify the code to capture and handle any errors from
file.Close()as follows:defer func() { if cerr := file.Close(); cerr != nil { err = fmt.Errorf("error closing file: %w", cerr) } }()This ensures that any errors during file closure are not overlooked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Makefile (2 hunks)
- commands_generate.go (7 hunks)
- commands_test.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 GitHub Check: codecov/patch
commands_generate.go
[warning] 177-177: commands_generate.go#L177
Added line #L177 was not covered by tests
[warning] 186-186: commands_generate.go#L186
Added line #L186 was not covered by tests
[warning] 190-190: commands_generate.go#L190
Added line #L190 was not covered by tests
[warning] 194-194: commands_generate.go#L194
Added line #L194 was not covered by tests
[warning] 201-202: commands_generate.go#L201-L202
Added lines #L201 - L202 were not covered by tests
🔇 Additional comments (6)
commands_test.go (4)
5-5: LGTM: Import addition for JSON decodingThe addition of the
encoding/jsonimport is appropriate for the new JSON decoding functionality in the test cases.
238-249: Excellent enhancement of JSON schema test coverageThe updates to the "--json-schema global" test case significantly improve the validation of the generated JSON schema. The new assertions and JSON decoding step ensure that the schema contains the expected elements and is structurally valid.
Well done on enhancing the test robustness!
259-265: Good addition: Test case for v2 JSON schemaThe new test case for "--json-schema v2" is a valuable addition to the test suite. It appropriately checks for v2-specific elements in the generated schema, enhancing the overall test coverage.
Line range hint
1-466: Overall: Excellent improvements to JSON schema test coverageThe changes in this file significantly enhance the test coverage for JSON schema generation. The addition of more detailed assertions and the new test case for v2 schema demonstrate a commitment to robust testing.
A few points to consider for future improvements:
- Consider adding more edge cases or error scenarios to further strengthen the test suite.
- As the JSON schema functionality evolves, ensure that the tests are updated accordingly to maintain comprehensive coverage.
Great work on improving the test suite!
commands_generate.go (2)
21-22: Good use of a constant for template pathReplacing the hardcoded string with the
pathTemplatesconstant enhances code maintainability and readability.
80-83: Proper error handling when loading templatesThe code correctly handles errors when loading templates using
fs.Subwith the newpathTemplatesconstant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (17)
examples/crond.yaml (4)
Line range hint
1-7: Excellent update to the schema reference and global settings.The schema reference update aligns well with the PR objectives for auto-versioning. The global settings look appropriate, with a sensible default for 'initialize'.
Consider adding a comment explaining the purpose of setting 'initialize' to false globally, as it might not be immediately clear to all users why this is the default.
Line range hint
9-12: Good default profile setup, but consider production readiness.The default profile provides a solid base for other profiles to inherit from. The use of a template in the lock path is particularly clever for profile-specific locks.
For production environments, consider using more permanent locations for the repository and lock files instead of '/tmp'. This directory is typically cleared on system reboot, which could lead to data loss or inconsistencies.
Line range hint
14-48: Well-structured user profiles with good use of inheritance and scheduling.The user profiles are nicely set up, inheriting from the default profile and overriding settings as needed. The staggered schedules for backups and checks are good for load distribution.
To improve clarity, consider adding comments to explain the reasoning behind the different schedules for each user. This would help future maintainers understand the intended backup and check frequency for each profile.
Line range hint
50-63: Consider adding context for the commented-out root profile.The commented-out root profile includes system-level permissions and a post-backup command, suggesting it's intended for more privileged operations.
To improve maintainability, consider adding a comment above this section explaining why it's currently commented out and under what circumstances it might be used. This would help future users or maintainers understand the purpose of this profile and when it might be appropriate to enable it.
docs/content/configuration/jsonschema/index.md (3)
Line range hint
20-58: Good updates to the vscode usage examplesThe YAML and JSON examples have been correctly updated to use the new unified schema URL, which is excellent. This consistency will help users implement the schema correctly in their projects.
However, I noticed that the TOML example still uses a versioned schema URL. While this might be intentional due to limited JSON Schema support in TOML (as mentioned in the PR objectives), it could be helpful to add a brief explanation for this difference to avoid confusion.
Consider adding a note explaining why the TOML example uses a different URL, perhaps just after the TOML code block.
60-87: Excellent addition of the Versioned URLs sectionThis new section provides crucial information about the versioning system for the schemas. It clearly explains the differences between universal and specific version schemas, which is very helpful for users who might need to use a particular version.
The inclusion of both config version and restic version URLs is comprehensive and aligns well with the PR objectives of implementing auto-versioning.
One minor suggestion for improvement: Consider adding a brief explanation of when a user might need to use these versioned URLs directly, as opposed to the main unified schema URL. This could help users understand the use cases for these specific versions.
🧰 Tools
🪛 LanguageTool
[typographical] ~63-~63: If the word ‘Which’ starts a question, add a question mark (“?”) at the end of the sentence.
Context: ...ed depends on config and restic version. You may use the URLs directly if you ne...(WHAT_NOUNPHRASE_QUESTION_MARK)
[grammar] ~72-~72: Did you mean “setting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... known restic versions and may allow to set flags that are not supported by a parti...(ALLOW_TO)
63-63: Addressing LanguageTool suggestions
The suggestion to add a question mark after "Which" on line 63 can be ignored. The sentence is not a question but a statement explaining which URL is selected based on config and restic version.
On line 72, the phrase "may allow to set flags" could be improved grammatically. Consider changing it to "may allow setting flags" or "may allow users to set flags" for better clarity and grammar.
Here's a suggested change for line 72:
-may allow to set flags that are not supported by a particular *restic* version. Descriptions +may allow setting flags that are not supported by a particular *restic* version. DescriptionsAlso applies to: 72-72
🧰 Tools
🪛 LanguageTool
[typographical] ~63-~63: If the word ‘Which’ starts a question, add a question mark (“?”) at the end of the sentence.
Context: ...ed depends on config and restic version. You may use the URLs directly if you ne...(WHAT_NOUNPHRASE_QUESTION_MARK)
examples/v2.yaml (6)
Line range hint
12-15: New 'full-backup' group enhances usability.The addition of the 'full-backup' group, which includes the 'root' and 'src' profiles, provides a convenient way to run multiple profiles together. This improvement aligns with the goal of enhancing usability and configuration management.
Consider adding a brief comment explaining the purpose and usage of this group for better documentation.
Line range hint
39-86: Comprehensive retention and forget policies added to 'root' profile.The 'root' profile has been significantly enhanced with detailed retention and forget policies. These additions provide fine-grained control over backup retention, which is a valuable feature for managing backups effectively.
To improve maintainability and ease of use, consider adding brief comments explaining the purpose of each retention and forget parameter, especially for less obvious options like 'keep-tag'.
Line range hint
87-114: 'self' profile enhanced with improved scheduling and error handling.The updates to the 'self' profile, including new scheduling parameters and error handling capabilities, significantly improve its flexibility and robustness. The use of environment variables and templating enhances configurability.
Consider using more descriptive names for the error handling commands. For example, instead of
echo restic returned an error, you might uselog_restic_erroror similar, assuming you have such a script or function available.
Line range hint
115-135: New 'prom' profile adds valuable Prometheus integration.The addition of the 'prom' profile with Prometheus integration is a significant enhancement, allowing for improved monitoring and metrics collection. The use of labels and file saving options provides flexibility in handling metrics.
To further improve the profile, consider adding a comment explaining the purpose of the 'prometheus-push' URL and any requirements for setting up the Prometheus endpoint.
Line range hint
149-173: 'src' profile enhanced with additional controls and actions.The updates to the 'src' profile, including additional backup checks and pre/post-backup commands, provide more granular control over the backup process. The use of templating in the commands enhances flexibility and customisation.
Consider grouping related configuration items together for improved readability. For example, you might group all 'run-before' commands under a single key using a YAML list.
Line range hint
214-228: 'escape' profile improved to handle complex file paths.The updates to the 'escape' profile, including the addition of paths with spaces and special characters, significantly improve its ability to handle real-world scenarios with complex file naming. The use of both quoted and unquoted strings effectively demonstrates different YAML syntax options.
To enhance clarity, consider adding a brief comment explaining the purpose of this profile and why these specific paths are included. This would help other developers understand the intent behind these configurations.
docs/content/configuration/variables.md (4)
Line range hint
18-72: Comprehensive update to template variables sectionThe expanded table of pre-defined variables and the addition of new variables (.BinaryDir, .OS, .Arch) greatly enhance the documentation. The examples for using the .Now variable are particularly helpful.
To further improve clarity, consider adding a brief example demonstrating the use of the new .OS and .Arch variables in a configuration snippet.
Line range hint
74-456: Excellent expansion of examples and addition of hand-made variablesThe inclusion of examples in multiple configuration formats (TOML, YAML, HCL, JSON) is extremely helpful for users working with different file types. The explanation of hand-made variables adds valuable information on creating custom variables.
To enhance the Windows path example, consider adding a brief explanation of why backslashes need to be escaped in Windows paths within string literals. This would help users understand the reasoning behind the different options presented.
Line range hint
458-594: Valuable addition of runtime variable expansion sectionThe new section on runtime variable expansion provides essential information on the differences between template and runtime variables. The example effectively demonstrates the practical use of runtime variables in a configuration file.
To further enhance this section, consider adding a brief note on the performance implications or use cases where runtime variables might be preferred over template variables. This could help users make informed decisions when choosing between the two types of variables.
Line range hint
595-598: Improved tip on escaping dollar signsThe update from "hint" to "tip" and the addition of a Windows-specific example ($RECYCLE.BIN) enhance the practical value of this information. This change will be particularly helpful for Windows users.
For consistency, consider using the same formatting for the "tip" notice as used in other parts of the documentation. This will ensure a uniform style throughout the document.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- Makefile (1 hunks)
- commands_test.go (3 hunks)
- docs/content/configuration/jsonschema/index.md (4 hunks)
- docs/content/configuration/variables.md (1 hunks)
- examples/crond.yaml (1 hunks)
- examples/dev.yaml (1 hunks)
- examples/linux.yaml (1 hunks)
- examples/v2.yaml (1 hunks)
- examples/windows.yaml (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/dev.yaml
- examples/linux.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- commands_test.go
🧰 Additional context used
🪛 Markdownlint
docs/content/configuration/jsonschema/index.md
13-13: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
🪛 LanguageTool
docs/content/configuration/jsonschema/index.md
[typographical] ~63-~63: If the word ‘Which’ starts a question, add a question mark (“?”) at the end of the sentence.
Context: ...ed depends on config and restic version. You may use the URLs directly if you ne...(WHAT_NOUNPHRASE_QUESTION_MARK)
[grammar] ~72-~72: Did you mean “setting”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun.
Context: ... known restic versions and may allow to set flags that are not supported by a parti...(ALLOW_TO)
🪛 yamllint
examples/windows.yaml
[error] 1-1: wrong new line character: expected \n
(new-lines)
🔇 Additional comments (4)
docs/content/configuration/jsonschema/index.md (2)
11-16: Excellent update to the Schema URL section!The changes in this section align perfectly with the PR objectives. The introduction of a single, unified schema URL ({{< absolute "jsonschema/config.json" >}}) that detects config and restic versions is a significant improvement. This change simplifies the documentation and enhances user experience by providing a single entry point for all schema versions.
The explanation of the new schema's functionality is clear and concise, which will help users understand the auto-versioning feature.
🧰 Tools
🪛 Markdownlint
13-13: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
13-13: Regarding Markdownlint hint: Emphasis used instead of a headingThe Markdownlint tool has flagged the use of bold text for the schema URL. However, in this context, the bold text is appropriate and doesn't require a heading. The emphasis helps to highlight the important URL without disrupting the document structure.
This hint can be safely ignored as the current formatting serves the intended purpose well.
🧰 Tools
🪛 Markdownlint
13-13: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
examples/v2.yaml (1)
1-1: Schema reference update aligns with PR objectives.The change from
config-2.jsontoconfig.jsonin the schema reference URL supports the goal of implementing a unified schema. This modification enhances maintainability by providing a consistent URL that won't require future updates for version changes.docs/content/configuration/variables.md (1)
Line range hint
9-16: Excellent addition of variable types explanationThe new section clearly distinguishes between template variables and runtime variables, providing crucial context for users. This addition significantly enhances the documentation by explaining when each type of variable is expanded during the configuration process.
Rerun of #195 after it emerges that yaml now supports our global file.
original description:
versionandglobal.restic-versionproperties (see also SchemaStore).generate --config-reference filenameadditionalPropertiesinstead of via regex matching with negative lookbehind)The is the first step to add the schema to the schemastore catalog (we need a single schema for this) and is also more convenient if the URL doesn't need to be changed 😀