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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Dockerfile for community/mercari-grpc-federation #1022

Merged
merged 7 commits into from
Jan 24, 2024

Conversation

goccy
Copy link
Contributor

@goccy goccy commented Jan 24, 2024

Hi @mfridman , Thank you for adding the plugin the other day 馃槃

I tried to use our plugin and found that the protoc-gen-grpc-federation command depends on the go command. This is because we use the imports package to format generated code.

So, I fixed Dockerfile to allow the use of the go command. Please check it.

The following is the error output.

: err: go command required, not found: exec: "go": executable file not found in $PATH: stderr:

The following error occurs if GOROOT ENV is not set, so that setting is also added.

: err: exit status 2: stderr: go: cannot find GOROOT directory: 'go' binary is trimmed and GOROOT is not set

@CLAassistant
Copy link

CLAassistant commented Jan 24, 2024

CLA assistant check
All committers have signed the CLA.

@mfridman
Copy link
Member

Ouch, I didn't realize this plugin invoked the Go command.

Note, when using protogen it handles formatting with the go/printer package. Specifically here:

https://github.com/protocolbuffers/protobuf-go/blob/b70f02be537926b5693bb047189608a7f2c80994/compiler/protogen/protogen.go#L1126-L1129

Having said all that, I'd prefer the plugin to be updated so it didn't need any dependencies. We enforce a strict contract for the plugins we maintain, i.e., they must operate entirely with the CodeGeneratorRequest / CodeGeneratorResponse and not need any external tools or files to function.

Sorry about that, I should have caught this earlier.

@@ -0,0 +1 @@
h1:d0P6WfW6ve00/zKIclE7fO8vZGCrQGOZnkhw9papifI=
Copy link
Member

Choose a reason for hiding this comment

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

This is the sum of the generated code from the sample image

// Code generated by protoc-gen-grpc-federation. DO NOT EDIT!
package gen

import (
	"context"
	"fmt"
	"io"
	"log/slog"
	"runtime/debug"

	"github.com/google/cel-go/cel"
	celtypes "github.com/google/cel-go/common/types"
	grpcfed "github.com/mercari/grpc-federation/grpc/federation"
	grpcfedcel "github.com/mercari/grpc-federation/grpc/federation/cel"
	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/trace"
	grpccodes "google.golang.org/grpc/codes"
	grpcstatus "google.golang.org/grpc/status"
)

// Federation_GetPostResponseArgument is argument for "federation.GetPostResponse" message.
type Federation_GetPostResponseArgument[T any] struct {
	Id     string
	Client T
}

// FederationServiceConfig configuration required to initialize the service that use GRPC Federation.
type FederationServiceConfig struct {
	// Resolver provides an interface to directly implement message resolver and field resolver not defined in Protocol Buffers.
	// If this interface is not provided, an error is returned during initialization.
	Resolver FederationServiceResolver // required
	// ErrorHandler Federation Service often needs to convert errors received from downstream services.
	// If an error occurs during method execution in the Federation Service, this error handler is called and the returned error is treated as a final error.
	ErrorHandler grpcfed.ErrorHandler
	// Logger sets the logger used to output Debug/Info/Error information.
	Logger *slog.Logger
}
[...]

@mfridman mfridman merged commit e22244e into bufbuild:main Jan 24, 2024
4 checks passed
@goccy
Copy link
Contributor Author

goccy commented Jan 25, 2024

@mfridman OK, Thank you for your reviewing. I will fix this in the next update. BTW, I knew about those formatters, but they did not match our use case 馃槄 . ( They cannot optimize the list of import packages )

@mfridman
Copy link
Member

For my curiosity, what do you mean by "optimize the list of import packages"?

But thanks for your understanding, once this is resolved please let us know so we can flip this config and resume pulling the latest versions, (it is currently disabled):

@goccy
Copy link
Contributor Author

goccy commented Jan 25, 2024

For my curiosity, what do you mean by "optimize the list of import packages"?

The packages used in the results generated by the gRPC Federation vary depending on the context. Therefore, a list of necessary and sufficient packages are predefined, and unnecessary packages are removed by the formatter.

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.

None yet

4 participants