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.

Sign up for GitHub

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

Jump to bottom

Fix/executiondata proto pkg #1353

Merged
merged 3 commits into from
Oct 17, 2023
Merged

Fix/executiondata proto pkg #1353

merged 3 commits into from
Oct 17, 2023

Conversation

treethought
Copy link
Contributor

@treethought treethought commented Jul 22, 2023 •
edited by peterargue
Loading

Closes: #???

Description

Fixes the package name for executiondata protobuf file from access -> executiondata.
Related to #1275 and onflow/flow-go-sdk#378 and needed for onflow/flow-go-sdk#417

Previously, with package access, the executiondata types could not be imported in generated go code.

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@koko1123
Copy link
Contributor

Hey this is a good idea because of the access package name overlap, but the intent behind the execution data APi was to be a subset of the Access API. The generated go files will break the flow-go implementation, so we have two possible routes around it:

  1. Modify the corresponding handlers in flow-go to avoid copying mutex values
  2. re-generate the go files with the older version of buf (in go.mod of this repo)

I suggest making a PR in flow-go for 1., then merging this PR, then updating the flow-go PR with the merged PR hash as a dependency

@peterargue
Copy link
Contributor

I think we should go ahead and make this change before the api is widely used.

@treethought can you pull in master and regenerate the files?

peterargue
peterargue reviewed Oct 16, 2023
View reviewed changes
protobuf/README.md Outdated
@@ -11,6 +11,7 @@ Check out the [Flow Access API specification](/docs/content/access-api.md).
You can use [buf](https://github.com/bufbuild/buf) to generate gRPC client stubs in a variety of languages.
Please make sure you have `protoc-gen-go-grpc` installed, for example using command
```shell script
go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.28
Copy link
Contributor

@peterargue peterargue Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not opposed to making this change, but I think we should do it as a separate PR.

Are you able to generate the files using the existing setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had added that line for what I used since I couldn't find what version was being used, but just found via #905 (comment) that it seems to be set in the flow-go makefile as v1.3.2

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok. thanks for fixing the docs!

@peterargue
Copy link
Contributor

FYI @nvdtf, @Guitarheroua

@peterargue peterargue merged commit 5d0f9b6 into onflow:master Oct 17, 2023
1 check passed
This was referenced Oct 17, 2023
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterargue peterargue peterargue approved these changes

Assignees
No one assigned
Labels
Access API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@treethought @koko1123 @peterargue