Add --timestamp and --detail options for backup-info command. #34
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Details about object filtering are presented as follows, depending on the active filtering type: * include-table / exclude-table: a comma-separated list of fully-qualified table names in the format <schema>.<table>; * include-schema / exclude-schema: a comma-separated list of schema names; * if no object filtering was used, the value is empty.
Update backups structure in prepare script.
The details are presented as follows, depending on the active filtering type: * include-table / exclude-table: a comma-separated list of fully-qualified table names in the format <schema>.<table>; * include-schema / exclude-schema: a comma-separated list of schema names; * if no object filtering was used, the value is empty.
woblerr
added
documentation
There was a problem hiding this comment.
Pull Request Overview
Adds --timestamp and --detail options to the backup-info command. The --timestamp option displays backup chains including dependent backups, while the --detail option adds an "object filtering details" column showing specific table or schema names for filtered backups.
- Added
--timestampoption to display backup chains for a specific backup with dependent backups - Added
--detailoption to show object filtering details in a new column - Updated tests and documentation to reflect the new functionality
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gpbckpconfig/struct.go | Added GetObjectFilteringDetails method to format filtering details as comma-separated strings |
| gpbckpconfig/struct_test.go | Added comprehensive tests for the new GetObjectFilteringDetails method |
| cmd/backup_info.go | Implemented the new --timestamp and --detail command-line options with validation |
| cmd/constants.go | Added constant for the new --detail flag |
| e2e_tests/scripts/run_tests/run_backup-info.sh | Updated test suite to include backup chain and detail column tests |
| e2e_tests/scripts/prepare/prepare_gpdb_backups.sh | Modified backup setup to use schema-based filtering for better test coverage |
| e2e_tests/README.md | Minor documentation update removing WAL archiving reference |
| COMMANDS.md | Updated documentation with detailed examples of the new options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
+154
to
+166
| func (backupConfig BackupConfig) GetObjectFilteringDetails() string { | ||
| filter, _ := backupConfig.GetObjectFilteringInfo() | ||
| switch filter { | ||
| case objectFilteringIncludeTable: | ||
| return strings.Join(backupConfig.IncludeRelations, ", ") | ||
| case objectFilteringExcludeTable: | ||
| return strings.Join(backupConfig.ExcludeRelations, ", ") | ||
| case objectFilteringIncludeSchema: | ||
| return strings.Join(backupConfig.IncludeSchemas, ", ") | ||
| case objectFilteringExcludeSchema: | ||
| return strings.Join(backupConfig.ExcludeSchemas, ", ") | ||
| default: | ||
| return "" |
There was a problem hiding this comment.
The error from GetObjectFilteringInfo() is being ignored. Consider handling the error or adding a comment explaining why it's safe to ignore it in this context.
Suggested change
| func (backupConfig BackupConfig) GetObjectFilteringDetails() string { | |
| filter, _ := backupConfig.GetObjectFilteringInfo() | |
| switch filter { | |
| case objectFilteringIncludeTable: | |
| return strings.Join(backupConfig.IncludeRelations, ", ") | |
| case objectFilteringExcludeTable: | |
| return strings.Join(backupConfig.ExcludeRelations, ", ") | |
| case objectFilteringIncludeSchema: | |
| return strings.Join(backupConfig.IncludeSchemas, ", ") | |
| case objectFilteringExcludeSchema: | |
| return strings.Join(backupConfig.ExcludeSchemas, ", ") | |
| default: | |
| return "" | |
| func (backupConfig BackupConfig) GetObjectFilteringDetails() (string, error) { | |
| filter, err := backupConfig.GetObjectFilteringInfo() | |
| if err != nil { | |
| return "", err | |
| } | |
| switch filter { | |
| case objectFilteringIncludeTable: | |
| return strings.Join(backupConfig.IncludeRelations, ", "), nil | |
| case objectFilteringExcludeTable: | |
| return strings.Join(backupConfig.ExcludeRelations, ", "), nil | |
| case objectFilteringIncludeSchema: | |
| return strings.Join(backupConfig.IncludeSchemas, ", "), nil | |
| case objectFilteringExcludeSchema: | |
| return strings.Join(backupConfig.ExcludeSchemas, ", "), nil | |
| default: | |
| return "", nil |
Comment on lines
+151
to
+152
| err = checkCompatibleFlags(flags, timestampFlagName, | ||
| typeFlagName, tableFlagName, schemaFlagName, excludeFlagName, failedFlagName, deletedFlagName, detailFlagName) |
There was a problem hiding this comment.
The list of incompatible flags is long and repeated in the error message on line 154. Consider defining this as a constant slice to avoid duplication and improve maintainability.
Comment on lines
+74
to
+77
| if [ ! -n "${got_details}" ]; then | ||
| echo "[ERROR] Expected details column to be non-empty" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The condition [ ! -n \"${got_details}\" ] is equivalent to [ -z \"${got_details}\" ] which is more idiomatic for checking if a string is empty. Consider using the latter for better readability.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added
--detailoption forbackup-infocommand:Adds the
object filtering detailscolumn.When
--detailis set, each row shows:<schema>.<table>;Added
--timestampoption forbackup-infocommand:object filtering detailscolumn.