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

Return error messages from HTTP handlers in body #72

Merged
merged 1 commit into from Feb 9, 2019

Conversation

Projects
None yet
4 participants
@pcsx22
Copy link
Contributor

pcsx22 commented Jan 27, 2019

Added error message to the reponse packet

Signed-off-by: pcsx22 sushil.paneru1@gmail.com

Description

This PR resolves #71

Motivation and Context

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Impact to existing users

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@derek derek bot added the new-contributor label Jan 27, 2019

@rgee0

This comment has been minimized.

Copy link
Member

rgee0 commented Jan 27, 2019

Please add a closure keyword with issue reference to the description. Thanks.

https://help.github.com/articles/closing-issues-using-keywords/

@pcsx22

This comment has been minimized.

Copy link
Contributor Author

pcsx22 commented Jan 27, 2019

@rgee0 done!

@alexellis alexellis requested a review from LucasRoesler Jan 27, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Jan 27, 2019

Hi Sushil,

I've just checked your commit sign-off message. It uses your alias/username rather than your full name, but we require a real name.

Signed-off-by: pcsx22

Should be whatever your real name is.

You can fix this with:

git config --global user.name "Your Full Name Here"
git commit --amend -s
git push... 
@alexellis
Copy link
Member

alexellis left a comment

Sign-off message needs updating as per comment. Have asked @LucasRoesler to review

@pcsx22 pcsx22 force-pushed the pcsx22:master branch from 3870c03 to 1b1f95f Jan 28, 2019

@pcsx22

This comment has been minimized.

Copy link
Contributor Author

pcsx22 commented Jan 28, 2019

Updated the name as per Alex's comment

@pcsx22 pcsx22 closed this Jan 28, 2019

@pcsx22 pcsx22 reopened this Jan 29, 2019

@pcsx22

This comment has been minimized.

Copy link
Contributor Author

pcsx22 commented Jan 30, 2019

@LucasRoesler Hey, can you review this PR?

@LucasRoesler
Copy link
Member

LucasRoesler left a comment

Looks good. I only see one needed change.

Also, I think there is a typo in the PR description, it should close 71 not 72

if err != nil {
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(err.Error()))
} else {

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 30, 2019

Member

This block doesn't need to be in an else statement.

This comment has been minimized.

@pcsx22

pcsx22 Jan 31, 2019

Author Contributor

@LucasRoesler Do you mean you don't want the error check for marshalling? Shall I undo the changes?

This comment has been minimized.

@LucasRoesler

LucasRoesler Jan 31, 2019

Member

I just mean that it should look like this

		if err != nil {
			w.WriteHeader(http.StatusInternalServerError)
			w.Write([]byte(err.Error()))
			return
		} 

		w.Header().Set("Content-Type", "application/json")
		w.WriteHeader(http.StatusOK)
		w.Write(infoBytes)

so that it matches the style in other handlers

This comment has been minimized.

@pcsx22

pcsx22 Feb 2, 2019

Author Contributor

@pcsx22 pcsx22 force-pushed the pcsx22:master branch from 1b1f95f to 2b0174b Feb 2, 2019

Return error messages from HTTP handlers in body
Added error message to the reponse packet

Signed-off-by: Sushil Paneru <sushil.paneru1@gmail.com>

@pcsx22 pcsx22 force-pushed the pcsx22:master branch from 2b0174b to 0242ca4 Feb 2, 2019

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Feb 2, 2019

Looks good. I only see one needed change.

Also, I think there is a typo in the PR description, it should close 71 not 72

@pcsx22 this line in the PR description should still be updated but the PR looks good 👍

@alexellis
Copy link
Member

alexellis left a comment

Approved. Thank you @LucasRoesler @pcsx22 👍

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 2, 2019

The last thing I'd like to see is some output showing that this actually works. Then I'll merge 👍

Alex

@pcsx22

This comment has been minimized.

Copy link
Contributor Author

pcsx22 commented Feb 3, 2019

demo_main

@alexellis I have posted output of some commands. All other handler should work the same

@pcsx22 pcsx22 closed this Feb 3, 2019

@pcsx22 pcsx22 reopened this Feb 3, 2019

@pcsx22

This comment has been minimized.

Copy link
Contributor Author

pcsx22 commented Feb 6, 2019

@alexellis I think you forgot to merge this

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 9, 2019

Why do you keep opening and closing the PR?

@alexellis alexellis merged commit 932ee22 into openfaas-incubator:master Feb 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Feb 9, 2019

Thanks for the PR 👍

@pcsx22

This comment has been minimized.

Copy link
Contributor Author

pcsx22 commented Feb 9, 2019

@alexellis Mistakely I have been clicking "close and comment" button instead of comment button. Sorry for the confusion

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