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

Basic initialization using server proto files #1

Closed
wants to merge 16 commits into from
Closed

Basic initialization using server proto files #1

wants to merge 16 commits into from

Conversation

KAVAN-DESAI
Copy link
Member

  • I have generated auto generate files using the proto files that is node.proto and primitives.proto associated with server which is useful to work with the client.
  • And have created a basic client structure.

vincenzopalazzo
vincenzopalazzo requested changes May 20, 2022
View reviewed changes
.dart_tool/package_config.json Outdated Show resolved Hide resolved
.idea/libraries/Dart_Packages.xml Outdated Show resolved Hide resolved
.packages Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
@KAVAN-DESAI
Copy link
Member Author

Connected the Client to the GRPC server using the Client Certificates to authenticate the Client.

vincenzopalazzo
vincenzopalazzo requested changes Jun 2, 2022
View reviewed changes
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Good job, I make one first pass for the review, after these change I will do another one

.gitignore Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
pubspec.lock Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo changed the title Basic initialisation using server proto files Basic initialization using server proto files Jun 2, 2022
vincenzopalazzo
vincenzopalazzo requested changes Jun 4, 2022
View reviewed changes
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Overall is a good starting point! after the change we could merge it and start to work on this issue #2 because this PR use the grpc client, but not provide a library for the GRPC client

What do you think?

bin/client.dart Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
bin/client.dart Outdated Show resolved Hide resolved
@vincenzopalazzo vincenzopalazzo self-assigned this Jun 4, 2022
vincenzopalazzo
vincenzopalazzo requested changes Jun 6, 2022
View reviewed changes
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Very good improvement inside the code, the last changes, and this PR is ready to land in the main branch!

Excited to see the client's implementation.

bin/client.dart Outdated Show resolved Hide resolved
lib/src/generated/primitives.pbgrpc.dart Outdated Show resolved Hide resolved
vincenzopalazzo
vincenzopalazzo reviewed Jun 7, 2022
View reviewed changes
bin/client.dart Outdated Show resolved Hide resolved
vincenzopalazzo
vincenzopalazzo requested changes Jun 7, 2022
View reviewed changes
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Very good job, the client draft looks very good, I have a couple of questions that I would like to discuss, but we are almost ready to land this PR on main!

bin/client.dart Outdated Show resolved Hide resolved
example/GRPC_client_example.dart Outdated Show resolved Hide resolved
example/GRPC_client_example.dart Outdated Show resolved Hide resolved
example/GRPC_client_example.dart Outdated Show resolved Hide resolved
lib/src/GRPC_client.dart Outdated Show resolved Hide resolved
lib/src/GRPC_client.dart Outdated Show resolved Hide resolved
lib/src/GRPC_client.dart Outdated Show resolved Hide resolved
lib/src/GRPC_client.dart Outdated Show resolved Hide resolved
lib/src/GRPC_client.dart Outdated Show resolved Hide resolved
vincenzopalazzo
vincenzopalazzo requested changes Jun 10, 2022
View reviewed changes
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

looks very good, only a few other minors comments

lib/cln_grpc.dart Outdated Show resolved Hide resolved
lib/src/grpc_client.dart Outdated Show resolved Hide resolved
lib/src/grpc_client.dart Outdated Show resolved Hide resolved
@vincenzopalazzo
Copy link
Member

There is two basic errors here:

  • you fork the repo and continue to push on your main, usually it is suggested to create a new branch for each feature
  • Your PR contains some merge requests and it is not suitable for how we manage this workflow, in particular, because we use rebase over the merge.

You can read more about the merge request here https://stackoverflow.com/questions/804115/when-do-you-use-git-rebase-instead-of-git-merge

I'm closing this because I push your change here with this commit where you are still the author a3b7e91

@vincenzopalazzo vincenzopalazzo linked an issue Jun 11, 2022 that may be closed by this pull request
provide a GRPC client implementation #2
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincenzopalazzo vincenzopalazzo vincenzopalazzo requested changes

Assignees

@vincenzopalazzo vincenzopalazzo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

provide a GRPC client implementation
2 participants
@KAVAN-DESAI @vincenzopalazzo