Skip to content
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

rule_type create: Add option to create multiple rule types at once or read all filed in directory #748

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented Aug 24, 2023

This changes the rule_type create command to be able to take an array of files
and create each of them. The intent is to provide a more agile way of using this
tool instead of having to do multiple command invocations.

If the passed argument is a directory, it'll read all the files in the directory and create rules out of it.

This changes the rule_type create command to be able to take an array of files
and create each of them. The intent is to provide a more agile way of using this
tool instead of having to do multiple command invocations.
This makes things simpler since we now can pass a directory
@JAORMX JAORMX changed the title rule_type create: Add option to create multiple rule types at once rule_type create: Add option to create multiple rule types at once or read all filed in directory Aug 24, 2023
@JAORMX JAORMX changed the title rule_type create: Add option to create multiple rule types at once or read all filed in directory rule_type create: Add option to create multiple rule types at once or read all filed in directory Aug 24, 2023
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

Some questions, but in general LGTM, feel free to merge

return nil
},
}

func validateFilesArg(files []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be already useful in util?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. I plan to migrate policy creation to a similar pattern. Mind if I move this in that PR instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, whatever is easier for you


if fi.IsDir() {
// expand directory
err := filepath.Walk(f, func(path string, info fs.FileInfo, err error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

the stdlib documentation seems to suggest that WalkDir might be faster, but at the same time I don't think it matters for our use-case

cmd/cli/app/rule_type/rule_type_create.go Show resolved Hide resolved
@JAORMX JAORMX merged commit cc1bae1 into main Aug 25, 2023
13 checks passed
@JAORMX JAORMX deleted the rule-type-create-files branch August 25, 2023 10:04
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

(I know it's late, I apparently forgot to hit "submit"... anyway, all my questions were advisory rather than blocking anyway.)

Comment on lines +53 to +55
if err != nil {
return fmt.Errorf("error getting grpc connection: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

(non-blocking curiousity) Why remove util.ExitNicelyOnError? We seem to use it in a bunch of places.

Comment on lines +99 to +101
if files == nil {
return fmt.Errorf("error: file must be set")
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to check len(files) > 0?

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.

3 participants