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

Feature: MeshWorks and Patternfiles #34

Merged
merged 4 commits into from
Feb 28, 2021
Merged

Feature: MeshWorks and Patternfiles #34

merged 4 commits into from
Feb 28, 2021

Conversation

leecalcote
Copy link
Member

Description

New feature support of MeshWorks and Patternfiles

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Utkarsh Srivastava <[email protected]>
Signed-off-by: Utkarsh Srivastava <[email protected]>
Signed-off-by: Utkarsh Srivastava <[email protected]>
@leecalcote leecalcote requested a review from a team February 1, 2021 19:10
@welcome
Copy link

welcome bot commented Feb 1, 2021

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

Copy link
Member

@Aisuko Aisuko left a comment

Choose a reason for hiding this comment

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

There may some advice please kindly check the comments

adapter/logger.go Show resolved Hide resolved
adapter/operations.go Outdated Show resolved Hide resolved
adapter/operations.go Outdated Show resolved Hide resolved
@Aisuko
Copy link
Member

Aisuko commented Feb 2, 2021

@leecalcote @Utkarsh-pro Could you guys write more detail about this feature?

@leecalcote
Copy link
Member Author

@leecalcote @Utkarsh-pro Could you guys write more detail about this feature?

Hi @Aisuko, yes, good request. Here we are - https://docs.google.com/document/d/1B2N78EdRiZF-yVo1-HY3syppwBBDumgMuYg6seD-AJ4/edit

@leecalcote leecalcote requested a review from a team February 3, 2021 17:35
adapter/operations.go Show resolved Hide resolved
api/grpc/grpc.go Show resolved Hide resolved
Signed-off-by: Utkarsh Srivastava <[email protected]>
@kumarabd
Copy link
Contributor

@Utkarsh-pro lets make a meshkit release to have proper versioning. We don't wanna stick to branch.

@mgfeller
Copy link
Contributor

@Utkarsh-pro out of interest: why are there 2 pb.go files (meshops.pb.go and meshops_grpc.pb.go)? They are also quite different. How was meshops_grpc.pb.go generated? Maybe Makefile needs to be adjusted?

@tangledbytes
Copy link
Contributor

@Utkarsh-pro out of interest: why are there 2 pb.go files (meshops.pb.go and meshops_grpc.pb.go)? They are also quite different. How was meshops_grpc.pb.go generated? Maybe Makefile needs to be adjusted?

@mgfeller new versions of protoc compiler spit out two files (if generating for grpc). One if the file is solely responsible for proto files serialization and deserialization while the other file contains the code for grpc (like implementing methods, interfaces etc.).

We don't have to change the makefile in my opinion, the new protoc will handle that automatically for us.

@mgfeller
Copy link
Contributor

@Utkarsh-pro out of interest: why are there 2 pb.go files (meshops.pb.go and meshops_grpc.pb.go)? They are also quite different. How was meshops_grpc.pb.go generated? Maybe Makefile needs to be adjusted?

@mgfeller new versions of protoc compiler spit out two files (if generating for grpc). One if the file is solely responsible for proto files serialization and deserialization while the other file contains the code for grpc (like implementing methods, interfaces etc.).

We don't have to change the makefile in my opinion, the new protoc will handle that automatically for us.

@Utkarsh-pro thanks for the feedback. Referring to meshery/meshery#2454, I had to install the latest plugin according to https://grpc.io/docs/languages/go/quickstart/ (I was probably using the github version superseded by this one). I had to update the Makefile as well:
protoc -I meshes/ meshes/meshops.proto --go-grpc_out=./meshes/ --go_out=./meshes/
Please have a look at meshery/meshery#2454

@leecalcote leecalcote merged commit 0921c5f into master Feb 28, 2021
@leecalcote leecalcote deleted the meshworks branch February 28, 2021 14:13
@welcome
Copy link

welcome bot commented Feb 28, 2021

Thanks for your contribution to the Layer5 community! 🎉

Congrats!

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.

5 participants