-
Notifications
You must be signed in to change notification settings - Fork 5
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
Parsec GO Client Initial API #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started reviewing, I'll leave comments incrementally as they pop up to me.
A first issue is that some of the committed files don't look like they should be upstreamed: the ...
archive file, .vscode/
... There might be others (and I'm actually not sure if some are needed for a Go environment in the first place).
Another question to ask is whether we want the protobuf-generated code to be commited into the repo or generated on the fly when the build happens (that's how we do it in the Rust client). Actually, I'm not really sure whether that last part is possible in Go, I just noticed the files in the tree.
keyinfo.go
Outdated
KeyBits: ka.KeyBits, | ||
// KeyPolicy: , | ||
}, nil | ||
// TODO finish this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say leave this as is and it can be expanded/improved in other PRs! Similarly for other TODOs or FIXMEs
extra files you mentioned removed, but will look for others. On the protobuf, I made the deliberate decision to add them to source control to make life easier for users - ideally, they should just be able to do a go get github.com/parallaxsecond/parsec-client-go and be able to start coding. In go (AFAIK), we don't have ability to do custom build logic, so code generation would be difficult for api users. Additionally, they'd need to install protoc, which would make their lives more difficult. makefile contains functionality to rebuild the protobuf derived files and I suppose, we could put a check into the ci build that generates the protobuf code and checks the code we have in source control is the same and fail the build if not - that would be a check that the dev had correctly run generation on the version of the .protos in use. May get rid of the interim .proto files from git mind... |
dbea67b
to
8c0e599
Compare
Signed-off-by: Justin Cormack <justin.cormack@docker.com> Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
7fe396b
to
7671379
Compare
Signed-off-by: Dax McDonald <dax@rancher.com>
19ab34a
to
075bd89
Compare
Basic end to end test only. Virtually no unit test. There are some functions that do not work Only support unix peer authentication Signed-off-by: John 9e9 <jn9e9e9@gmail.com> set parsec-operations version to 0.6.0
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jn9e9 !!
Thanks a lot for your work, that is amazing. That will make all Parsec Go users really happy.
I think we agreed to merge your PR as it is (after the following steps, see below) and after that aim at producing smaller PRs on specific topics (ie improving testing, checking top-level API, etc).
About protobuf
As you said, if in Go there is no build script to be able to generate on-the-fly Go protobuf files then I think it's fine to have them checked in tree. Parsec follow the open-closed principle so that the contracts should never change in a breaking way: if the contracts generated in the Go client are older than those generated in the service, the client's operations should still work (well that's a goal anyway 😅).
I think it's nice to have a make
command to re-generate them so that when new contracts appear on the parsec-operations
submodule, one can pull latest master
from that submodule and generate the new contracts.
It would be nice that at any given time, the parsec-operations
submodule is in sync with the Generated Go files: a CI job could check for that.
Things to do before we can merge
- Remove all files not used or not needed. That would also be all that was previously in the repo but that you don't need. I think you did most of it already but just making sure of that 😃
- Grouping files into a more readable directory structure. I am not sure how possible it is in Go with the module system but it seems that we could do the following: one folder for everything related with testing (Go test files, bash scripts, Parsec config files, etc), one folder for the interface (containing the
parsec-operations
submodule, the generated Go files, the slim abstraction over them, subfolders for the wire protocol (request and response fields)) and one folder for the "proper" Go client code (the basic client and all logic above the interface). I think this is just an example but it would be nice to have some directory structure anyway so that it would be simpler to see what's going on and to contribute. - Cleanup the
README.md
file. In theLicense
section, you can remove the "this project uses the following crates" paragraph. It would also be awesome if it contained one section explaining the directory structure set up above and another one explaining the variousmake
commands: how to generate the protobuf files for example, how to compile, how to test, etc.
Sorry to ask for all that!! Happy to help with those tasks as well in any way you want.
edit: if that is easier for you, I think we don't mind if you just squash all your commits into one big one.
@hug-dev - happy to do those items. do you want the freshness check on parsec-operations doing this time round or shall i create an issue for that? on test files, will make sure all e2e stuff in once place - have a feeling that idiomatic go unit testing is in same folder - will double check and make sure that structure is a) idiomatic, b) clean and tidy, if that's OK? |
Great 😄
I think later should be better, so that it will be easier to review independently!
yep, all sounds good! |
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Done
Done - added directory structure, and basic make commands for protobuf, clean, compile, test (unit and e2e)
NP, think it looks cleaner now
Think that's all comments addressed (and sorted signoffs) |
…ent-go Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
Signed-off-by: John 9e9 <jn9e9e9@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's excellent work, thank you! The new structure is very clear and will be a nice base to work on for subsequent tasks. It's good to be merged on my part!
If you want, don't forget to add yourself as a Parsec contributor by making a PR on this file!
- [e2etest](https://github.com/parallaxsecond/parsec-client-go/tree/master/e2etest) End to End testing - Docker containers to fire up parsec and run end to end tests. Also used in CI end to end testing. | ||
- [interface](https://github.com/parallaxsecond/parsec-client-go/tree/master/interface) The Google Protocol Buffers basic client for communicating with the parsec daemon. This provides the underlying interface to parsec, but is not intended for client application use. | ||
- [parsec](https://github.com/parallaxsecond/parsec-client-go/tree/master/parsec) This is the public interface of the Parsec Go client. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes on the README! Looks really great and clear now 💯
@@ -0,0 +1,13 @@ | |||
# Interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice structure!
Initial version of parsec go API. This has only very limited testing and no inline documentation and only support for unix peer authenticator - see README.md for current status.
Quality checks on github ci tests failing.
Not necessarily expecting that this will be accepted but would appreciate comment on the API. (this is my best stab at the structure of the basic client api for now).
Partial implementation of #4