Skip to content

Refine Inspect queries and add CSVQ support for rule parsing #3595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

encima
Copy link
Member

@encima encima commented May 21, 2025

What kind of change does this PR introduce?

Feature

What is the current behavior?

Currently, inspect reports are quite modular and provide output where column titles can vary

What is the new behavior?

Refined queries to output in a similar format and reduced/combined work to minimise the number of output files produced (or commands needed)

Additional context

CSVQ has been added as a library to provide a table printout of some rules that are/are not met by the CSV files produced by report

image

@encima encima requested a review from a team as a code owner May 21, 2025 13:53
@encima encima force-pushed the feat/inspect-rules branch from 80c020e to e266458 Compare May 26, 2025 09:54
cmd/inspect.go Outdated
Comment on lines 110 to 116
inspectTotalIndexSizeCmd = &cobra.Command{
Use: "total-index-size",
Short: "Show total size of all indexes",
RunE: func(cmd *cobra.Command, args []string) error {
return total_index_size.Run(cmd.Context(), flags.DbConfig, afero.NewOsFs())
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better hide deprecated commands instead so it doesn't break existing users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we hide these typically? Do you return a recommendation of the new command or call the new command and print a deprecation warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli/cmd/db.go

Line 171 in b2ac658

Deprecated: "use \"db diff --use-migra --linked\" instead.\n",

Depends on the situation. I think in this case, it is fine to invoke new command and print warning.

If there's no close substitute, then we need to keep the old code around as well.

@@ -30,7 +29,7 @@ func Run(ctx context.Context, config pgconn.Config, fsys afero.Fs, options ...fu
return err
}
defer conn.Close(context.Background())
rows, err := conn.Query(ctx, TotalTableSizesQuery, reset.LikeEscapeSchema(utils.PgSchemas))
rows, err := conn.Query(ctx, TotalTableSizesQuery, reset.LikeEscapeSchema(utils.InternalSchemas))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure that users are not interested in the size of managed schemas, like auth.users table for eg?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for auth, yep. For others, less so because they are managed and should not be touched (so they may cause issues but nothing the user can resolve)

…s thrown when report directory cannot be made
@encima encima force-pushed the feat/inspect-rules branch from b9644ed to 9f4a035 Compare May 29, 2025 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants