-
Notifications
You must be signed in to change notification settings - Fork 444
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
add docker rest api agent #180
Conversation
This pull request introduces 8 alerts when merging 05a3ed4 into ae053b6 - view on LGTM.com new alerts:
|
05a3ed4
to
4d0ba69
Compare
This pull request introduces 2 alerts when merging 4d0ba69 into ae053b6 - view on LGTM.com new alerts:
|
4d0ba69
to
0d82c0b
Compare
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.
Besides, where are the testing cases?
|
||
|
||
class NodeStart(Resource): | ||
def put(self): |
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.
put --> POST
res['status'] = 'deleted' | ||
return res | ||
|
||
api.add_resource(QueryNetwork, '/api/v1/query_network') |
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.
Nodes as the class.
Each operation is as a method.
@app.route('/api/v1/nodes/<id>', methods=['GET', 'POST']) | ||
def operate_node(id): | ||
|
||
if request.method == 'POST': |
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.
Why do we need this special if check?
try: | ||
container.stop() | ||
except: | ||
res['code'] = 'Fail' |
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.
If you are using the same code snippet more than once, better wrap into some small method.
0d82c0b
to
dc47444
Compare
This pull request introduces 7 alerts when merging dc47444 into 24ace07 - view on LGTM.com new alerts:
|
20981f0
to
fea705e
Compare
# same as `docker run -dit yeasy/hyperledge-fabric:2.2.0 -e VARIABLES`` | ||
container = client.containers.run(request.form.get('img'), request.form.get('cmd'), detach=True, tty=True, stdin_open=True, name=request.form.get('name'), environment=env) | ||
except: | ||
res['code'] = 'Fail' |
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.
Better use const to define the status code.
b9bfed3
to
c184c04
Compare
@yeasy I have addressed the comments and updated the code base, please help review. Thanks! |
docker agent for api-agent hyperledger#121 * support for node creation, start, stop, restart, deletion * add crypto materials and integration test Signed-off-by: Dixing (Dex) Xu <dixingxu@gmail.com>
c184c04
to
c0b57be
Compare
docker agent for api-agent #121
Signed-off-by: Dixing (Dex) Xu dixingxu@gmail.com