gluster / python-gluster-mgmt-client

Python bindings for gluster
GNU General Public License v3.0
5 stars 3 forks source link

Added Auto-Codegeneration script and json API definitions #21

Open sidharthanup opened 6 years ago

sidharthanup commented 6 years ago

Added Auto-Codegeneration script and json API definitions for Peer and Volume Management, Geo-replication, Bitrot, Snapshot and Self-heal.

Signed-off-by: Sidharth Anupkrishnan sanupkri@redhat.com

sidharthanup commented 6 years ago

@phlogistonjohn The API definition files were hand written following the OpenAPI 3 specification using the REST API endpoints doc as the reference.

Madhu-1 commented 6 years ago

am I missing anything over here, where I can see the auto-generated code? @sidharthanup if this PR has only scripts, we also need to make sure auto-generated client code is also updated here, I see only scripts file, what about the auto-generated API's?

if we want to merge only scripts file, we should make sure auto-generated code is good to use

sidharthanup commented 6 years ago

@Madhu-1 Will add auto generated client library along with the necessary changes.

phlogistonjohn commented 6 years ago

The API definition files were hand written following the OpenAPI 3 specification using the REST API endpoints doc as the reference.

OK. The main concern I have about this approach is that we've gone from writing python code that could get out of sync with gd2's apis to writing json files that could get out of sync with gd2's apis. While I do think that this is a step in the right direction I think that what we really ought to be aiming for is a single source of truth. Either have the code in gd2 also generated from the api spec files OR have gd2 itself generate the api spec files. This isn't a reason to reject the PR, just something I think we need to be working toward over time.

Because you're editing these by hand, I do think that writing YAML would be preferred to editing json directly. Did you investigate using yaml at all? After all, all yaml can be converted to JSON, and it's generally easier to work with in an editor, etc.

sidharthanup commented 6 years ago

Did the changes.. @phlogistonjohn I had originally written it in YAML and as you said converted it to json because everyone here was using JSON. I can add the yaml as well. Also i do believe that getting the api definition spec files via GD2 endpoint requestbody and responsebody definitions is the way to go, I will be working on implementing a parser to do that next.

sidharthanup commented 6 years ago

@phlogistonjohn So I should remove the old code(attic directory) in the PR? Should'nt we wait till the code generation get's reviewed and stabilized/merged?

sac commented 6 years ago

am I missing anything over here, where I can see the auto-generated code? @sidharthanup if this PR has only scripts, we also need to make sure auto-generated client code is also updated here, I see only scripts file, what about the auto-generated API's?

if we want to merge only scripts file, we should make sure auto-generated code is good to use

  • can you paste any one of the autogenerate client file, I want to make sure the client function does all the basic validation required before sending the request to glusterd2

@Madhu-1 we need not commit the autogenerated code to the repository. While packaging a Makefile or a script will generate the code. If we include the autogenerated code in the repository and forget to keep it updated, it will be grossly out of sync. It is better to provide a script to generate it and document that.

phlogistonjohn commented 6 years ago

So I should remove the old code(attic directory) in the PR? Should'nt we wait till the code generation get's reviewed and stabilized/merged?

I would remove it yes. Simply moving it to a different sub-dir in the repo is just duplicating the work the VCS does for you IMO.

sidharthanup commented 6 years ago

Have updated the PR. Moved code and Re-organised the repo to have a definite directory structure.

sidharthanup commented 6 years ago

@sac Ack. Updated the changes.

sidharthanup commented 6 years ago

@phlogistonjohn @Madhu-1 Please review it again.