-
Notifications
You must be signed in to change notification settings - Fork 511
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
feat: Add functionality to export proto files #475
Conversation
Added a new function, `WriteProtoFiles` in `desc_source.go` which is used to generate .proto files. The process involves resolving symbols from the descriptor source and writing their definitions to a designated output directory. The corresponding flag `--proto-out` has been included in `grpcurl.go` to allow users to specify the directory path.
The code for file creation and error handling in desc_source.go has been refactored. Previously, the file closure operation was executed irrespective of whether the file was created successfully or not. Now, the file will only be closed if it was successfully created, improving error handling.
The command for exporting proto files and setting the output directory has been updated from 'proto-out' to 'proto-out-dir'. This change has been made both in the README and the grpcurl go file. This makes the command name more descriptive, accurately reflecting its functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me, @jhump wdyt?
The file close operation has been moved within the error handling of the 'PrintProtoFile' function. Previously, it was being executed before this function, now it's executed immediately after. Moreover, an additional close operation has been added after the function success ensuring the file is properly closed in all scenarios.
The grpcurl commands for exporting proto files and protoset files in the README are updated. The IP address has been changed to localhost and port number to '8787'. Also, the service name is adjusted to 'my.custom.server.Service'. Instructions for use of specific command options are added for enhanced clarity.
desc_source.go
Outdated
if f != nil { | ||
_ = f.Close() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to omit this block, if err is non-nil, f will be nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solved
desc_source.go
Outdated
_ = f.Close() | ||
} | ||
return fmt.Errorf("failed to create file %q: %v", filePath, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
immediately after this block, you should probably defer f.Close()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can't defer in a for loop.
See number 2: https://blog.learngoprogramming.com/gotchas-of-defer-in-go-1-8d070894cb01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to scope it sometimes:
for _, fd := range allFilesSlice {
fdFQName := fd.GetFullyQualifiedName()
dirPath := filepath.Dir(fdFQName)
outFilepath := filepath.Join(outProtoDirPath, dirPath)
if err := os.MkdirAll(outFilepath, 0755); err != nil {
return fmt.Errorf("failed to create directory %q: %v", outFilepath, err)
}
fileName := filepath.Base(fdFQName)
filePath := filepath.Join(outFilepath, fileName)
err := func() error {
f, err := os.Create(filePath)
if err != nil {
return fmt.Errorf("failed to create")
}
defer f.Close()
if err := pr.PrintProtoFile(fd, f); err != nil {
return fmt.Errorf("failed to write")
}
if err := f.Close(); err != nil {
return fmt.Errorf("failed to close")
}
return nil
}()
if err != nil {
return fmt.Errorf("file %q: %w", filePath, err)
}
}
return nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^ yeah this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ready
desc_source.go
Outdated
if err := pr.PrintProtoFile(fd, f); err != nil { | ||
_ = f.Close() | ||
return fmt.Errorf("failed to write file %q: %v", filePath, err) | ||
} | ||
_ = f.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You actually want to error check the close as well. Something like:
err = pr.PrintProtoFile(fd, f)
if err == nil {
err = f.Close()
}
// do the error check now
you also don't need the final close if you deferred close earlier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ready
The code responsible for error handling during file creation in the desc_source.go file has been streamlined. This modification simplifies the code by reducing unnecessary condition checks and redundant file closure action after an error has occurred.
@dragonsinth hello. Is there anything else to complete? |
The file writing process for protobuf files has been extracted into a new function called writeProtoFile(). This refactoring simplifies the main function. The code is cleaner and more manageable this way, improving maintainability and readability.
return fmt.Errorf("failed to write proto file: %v", err) | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do one more pass on this, I have a few changes:
- simplify filepath calculations and naming
- improve errors with filename and error wrapping
- 0777 so the system level umask can take effect
func writeProtoFile(outProtoDirPath string, fd *desc.FileDescriptor, pr *protoprint.Printer) error {
outFile := filepath.Join(outProtoDirPath, fd.GetFullyQualifiedName())
outDir := filepath.Dir(outFile)
if err := os.MkdirAll(outDir, 0777); err != nil {
return fmt.Errorf("failed to create directory %q: %w", outDir, err)
}
f, err := os.Create(outFile)
if err != nil {
return fmt.Errorf("failed to create proto file %q: %w", outFile, err)
}
defer f.Close()
if err := pr.PrintProtoFile(fd, f); err != nil {
return fmt.Errorf("failed to write proto file %q: %w", outFile, err)
}
return nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ready
Streamlined the writeProtoFile function in desc_source.go file. Simplified path calculations and improved error messages for file-creation functions, making it easier to trace the exact point of failure and enhance the debugging process.
Sorry for lack of reply. I've been on a family vacation. FWIW, looks good to me, too 👍. |
keen to use this feature too, when will we release it? |
Keen to use this feature too. Wonder how people deal with this without this feature |
Feature Description:
Add a new option --proto-out-dir to grpcurl that allows users to generate .proto files for listed, described, or invoked elements and their transitive dependencies.
Functionality:
When using the list and describe verbs, the listed or described elements and their transitive dependencies will be written as .proto files in the specified directory.
Proposed Implementation:
Add a new flag:
Use Case:
This feature will be useful for users who need to generate .proto files from gRPC services, especially when working with services that don't have readily available .proto files or when dealing with dynamically generated services.
Tested with the starlink gRPC server.
TEST: OK
closes #474
closes #206