Skip to content

Setting outgoing protocol#89

Merged
amaniyechuri merged 1 commit intodeep-compute:masterfrom
IdavalapatiRamanjaneyulu:master
May 10, 2019
Merged

Setting outgoing protocol#89
amaniyechuri merged 1 commit intodeep-compute:masterfrom
IdavalapatiRamanjaneyulu:master

Conversation

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread kwikapi/api.py
self._invoke_pre_call_hook(request)
result = request.fn(**request.fn_params)

protocol = self._find_request_protocol(request)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RamanjaneyuluIdavalapati What is this protocol used for??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jaswanth098 We use that protocol to serialise the response/result.
By default JSON protocol will be used to serialise and deserialise the incoming(client sends data to the server) and outgoing(server sends data to the client) data.

We can change this default protocol. Ref

But this protocol will be used to serialise and deserialise for both incoming and outgoing data.
I have supported here such that the user can change the protocol for outgoing data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

got it thanks for clarification

Comment thread kwikapi/api.py
self._invoke_pre_call_hook(request)
result = request.fn(**request.fn_params)

protocol = self._find_request_protocol(request)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

got it thanks for clarification

@amaniyechuri amaniyechuri merged commit 886daaa into deep-compute:master May 10, 2019
@ghost ghost removed the inprogress-status label May 10, 2019
@prashanthellina
Copy link
Copy Markdown
Member

@RamanjaneyuluIdavalapati Please undo this merge. We should not be using request.headers as a mechanism to signal to kwikapi code that the response protocol is raw type. For this, we can use request.response.headers (which is not there today). Please add headers as an abstract property to BaseResponse and implement support in TornadoResponse and DjangoResponse.

If the above is done, instead of _find_request_protocol, we can have _find_response_protocol which will examine request.response instead of request.

In addition, we can take all the headers mentioned in request.response.headers and set them in the raw response so they are sent back to the caller.

Please call me if this isn't clear.

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

@prashanthellina How do I undo/revert the merge and commit here? I have seen this https://stackoverflow.com/questions/2389361/undo-a-git-merge-that-hasnt-been-pushed-yet. This will work in local.

After doing the above should I commit again?

@prashanthellina
Copy link
Copy Markdown
Member

@RamanjaneyuluIdavalapati No idea. Haven't done that before.

@jaswanth098
Copy link
Copy Markdown
Contributor

@RamanjaneyuluIdavalapati you can see a revert option above beside the merge that amani has done

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

@jaswanth098 I tried it, but it is creating another commit.

@jaswanth098
Copy link
Copy Markdown
Contributor

Yes that is the revert commit that has to be applied in github, once a commit is made to the github it cannot be removed you can only make one more commit by removing all those changes that is what the above revert is about

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

@jaswanth098 @prashanthellina If that is the case (commit can't be deleted), let me implement response headers and give pull request instead of reverting.

It is because there is of no use with revert other than making duplicate commits.

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

@jaswanth098 @prashanthellina This is working https://stackoverflow.com/questions/448919/how-can-i-remove-a-commit-on-github to remove the commit. But it will remove only visibility of the commit from the Github. If you have the commit id then you can still access the commit.

@prashanthellina
Copy link
Copy Markdown
Member

@RamanjaneyuluIdavalapati That works.

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

IdavalapatiRamanjaneyulu commented May 13, 2019

@prashanthellina will work on this tomorrow. Do I have write access to this repo? If I have I can simply remove the commit. Not sure will this work(removing) for pull request also?

@prashanthellina
Copy link
Copy Markdown
Member

@RamanjaneyuluIdavalapati You have write access and that allows you to edit the code in the repo.

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

IdavalapatiRamanjaneyulu commented May 14, 2019

@prashanthellina I don't have permission to push directly. May be this is because the branch is protected.

Screenshot 2019-05-14 at 11 58 59 AM

@jaswanth098
Copy link
Copy Markdown
Contributor

@RamanjaneyuluIdavalapati No one has permission to push to master you should start a pull request to master branch from a different branch

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

IdavalapatiRamanjaneyulu commented May 14, 2019

@jaswanth098 Is this because of Github or we set up like this? If we set up like this can't we have permission for this push alone (Change the set up until this pull gets merge)?

If we can't able to change it then I will give a pull request from different branch. Not sure whether we can able to merge or not. Not sure will this delete the commits.

@jaswanth098
Copy link
Copy Markdown
Contributor

jaswanth098 commented May 14, 2019

@RamanjaneyuluIdavalapati This is something that we did intentionally because no one pushes some buggy code to master with a review
Right now I don't have permission to change this setting.
anyway this is an enhancement that you are gonna do right why don't you do it on top of it instead of doing a revert in the code

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

@jaswanth098 I have checked on my personal Github repository by pushing to different branch. When I tried to create a pull request it is saying no changes(even there are changes between commits) and not accepting the pull request.

why don't you do it on top of it instead of doing a revert in the code

You mean keep the commit as it is and proceed to implement further features?

@jaswanth098
Copy link
Copy Markdown
Contributor

@jaswanth098 I have checked on my personal Github repository by pushing to different branch. When I tried to create a pull request it is saying no changes(even there are changes between commits) and not accepting the pull request.

why don't you do it on top of it instead of doing a revert in the code

You mean keep the commit as it is and proceed to implement further features?

yes

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

anyway this is an enhancement that you are gonna do right

@jaswanth098 I have checked this on my personal Github repo. It worked there when I directly push to master. That is the reason I told this is working.

@IdavalapatiRamanjaneyulu
Copy link
Copy Markdown
Contributor Author

IdavalapatiRamanjaneyulu commented May 14, 2019

@jaswanth098 @prashanthellina This is working https://stackoverflow.com/questions/448919/how-can-i-remove-a-commit-on-github to remove the commit. But it will remove only visibility of the commit from the Github. If you have the commit id then you can still access the commit.

@jaswanth098 @prashanthellina Example for the above.
I have given 3 commits for the repo https://github.com/chowdaryidavalapati/removecommit.
But you can only find 2 because I have removed one. This will be removed from the history but still can be accessed if you have the commit id.
3rd commit: https://github.com/chowdaryidavalapati/removecommit/tree/bcafd782b338817cc9285fc7618d2f8b35922dfa

But anyway will not worry about this and will continue to implement further features as you suggested.
I have provide this because this may help in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants