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

Import base go file even if there are no basetypes required #418

Merged
merged 4 commits into from
Dec 9, 2022

Conversation

joshcarp
Copy link
Contributor

@joshcarp joshcarp commented Dec 8, 2022

If a service solely requires types from other packages the result is that the connect services don't have access to the filedescriptorsets. This causes a lot of issues such as server reflection failing.

This PR blank imports the base go file that should always exist

For the proto file:

// service/eliza.proto
syntax = "proto3";
package service;
import "models/eliza.proto";

service ElizaService {
  rpc Say(ext.SayRequest) returns (ext.SayResponse) {}
}

// models/eliza.proto
syntax = "proto3";
package ext;

message SayRequest {
  string sentence = 1;
}
message SayResponse {
  string sentence = 1;
}

The following diff occurs in the generated eliza.connect.go code

package serviceconnect

import (
	context "context"
	errors "errors"
	models "github.com/bufbuild/connect-demo/internal/gen/buf/connect/demo/eliza/v1/models"
+	_ "github.com/bufbuild/connect-demo/internal/gen/buf/connect/demo/eliza/v1/service"
	connect_go "github.com/bufbuild/connect-go"
	http "net/http"
	strings "strings"
)

There is no change if the base types are already in the service/eliza.proto (the way we recommend using proto files)

Closes TCN-786

@joshcarp joshcarp requested a review from akshayjshah December 8, 2022 19:29
@joshcarp joshcarp changed the title Import base go file even if there are no basetypes required in there Import base go file even if there are no basetypes required Dec 8, 2022
@pkwarren
Copy link
Contributor

pkwarren commented Dec 9, 2022

Change looks good to me - should we regenerate the code in connect-go with this plugin?

Not sure if it is worth it, but we might consider creating a small example for this under internal/proto and a test which verifies the global registry contains the service and all messages after importing the generated connect code.

Copy link
Member

@akshayjshah akshayjshah left a comment

Choose a reason for hiding this comment

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

LGTM - nice fix, @joshcarp! As @pkwarren suggested, please do add a test case to prevent us from breaking this again in the future.

Copy link
Contributor

@pkwarren pkwarren left a comment

Choose a reason for hiding this comment

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

nice work!

@joshcarp joshcarp merged commit 528158c into main Dec 9, 2022
@joshcarp joshcarp deleted the import-basefile branch December 9, 2022 23:10
renovate bot referenced this pull request in open-feature/flagd Jan 12, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/bufbuild/connect-go](https://github.com/bufbuild/connect-go)
| require | minor | `v1.3.1` -> `v1.4.1` |

---

### Release Notes

<details>
<summary>bufbuild/connect-go</summary>

###
[`v1.4.1`](https://github.com/bufbuild/connect-go/releases/tag/v1.4.1)

[Compare
Source](https://github.com/bufbuild/connect-go/compare/v1.4.0...v1.4.1)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Bugfixes

- Ensure server reflection can always access protobuf descriptors by
[@&#8203;joshcarp](https://github.com/joshcarp) in
[https://github.com/bufbuild/connect-go/pull/418](https://github.com/bufbuild/connect-go/pull/418)
- Don't clobber custom User-Agents by
[@&#8203;akshayjshah](https://github.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/423](https://github.com/bufbuild/connect-go/pull/423)

**Full Changelog**:
bufbuild/connect-go@v1.4.0...v1.4.1

###
[`v1.4.0`](https://github.com/bufbuild/connect-go/releases/tag/v1.4.0)

[Compare
Source](https://github.com/bufbuild/connect-go/compare/v1.3.2...v1.4.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Enhancements

- Improve panic message on invalid handler returns by
[@&#8203;akshayjshah](https://github.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/415](https://github.com/bufbuild/connect-go/pull/415)
- Add support for Connect-Protocol-Version header by
[@&#8203;akshayjshah](https://github.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/416](https://github.com/bufbuild/connect-go/pull/416)

#### Migration Notes

As a consequence of
[https://github.com/bufbuild/connect-go/pull/416](https://github.com/bufbuild/connect-go/pull/416),
`connect-go` servers exposed to web browsers may need to amend their
CORS configuration to add `Connect-Protocol-Version` to
`Access-Control-Allow-Headers`. The [pull request
description](https://github.com/bufbuild/connect-go/pull/416) explains
the motivation for this additional header.

**Full Changelog**:
bufbuild/connect-go@v1.3.2...v1.4.0

###
[`v1.3.2`](https://github.com/bufbuild/connect-go/releases/tag/v1.3.2)

[Compare
Source](https://github.com/bufbuild/connect-go/compare/v1.3.1...v1.3.2)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

##### Bugfixes

- Send gRPC error metadata only as HTTP trailers by
[@&#8203;akshayjshah](https://github.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/410](https://github.com/bufbuild/connect-go/pull/410)

**Full Changelog**:
bufbuild/connect-go@v1.3.1...v1.3.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/open-feature/flagd).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC45Ny42IiwidXBkYXRlZEluVmVyIjoiMzQuOTcuNiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
If a service solely requires types from other packages the result is
that the connect services don't have access to the filedescriptorsets.
This causes a lot of issues such as server reflection failing.

This PR blank imports the base go file that should always exist

For the proto file:

```proto
// service/eliza.proto
syntax = "proto3";
package service;
import "models/eliza.proto";

service ElizaService {
}

// models/eliza.proto
syntax = "proto3";
package ext;

message SayRequest {
  string sentence = 1;
}
message SayResponse {
  string sentence = 1;
}
```

The following diff occurs in the generated `eliza.connect.go` code
```diff
package serviceconnect

import (
	context "context"
	errors "errors"
	models "github.com/bufbuild/connect-demo/internal/gen/buf/connect/demo/eliza/v1/models"
+	_ "github.com/bufbuild/connect-demo/internal/gen/buf/connect/demo/eliza/v1/service"
	connect_go "github.com/bufbuild/connect-go"
	http "net/http"
	strings "strings"
)

```

There is no change if the base types are already in the
`service/eliza.proto` (the way we recommend using proto files)

Closes TCN-786
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