Skip to content
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

Adding ReasonForError usage: #384

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@acornies
Copy link
Member

acornies commented Mar 8, 2019

To address #362 by mapping more appropriate http error responses for secrets API.

Description

  • Required update k8s.io/apimachinery to 1.9
  • Added processErrorReasons mapping private function

Motivation and Context

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

How Has This Been Tested?

TODO

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)

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.
Adding ReasonForError usage:
- Required update k8s.io/apimachinery to 1.9
- Added processErrorReasons mapping private function

Signed-off-by: Andrew Cornies <acornies@gmail.com>
@@ -54,8 +54,7 @@ func getSecretsHandler(kube typedV1.SecretInterface, w http.ResponseWriter, r *h
selector := fmt.Sprintf("%s=%s", secretLabel, secretLabelValue)
res, err := kube.List(metav1.ListOptions{LabelSelector: selector})
if err != nil {
w.WriteHeader(http.StatusInternalServerError)
log.Printf("Secrets query error: %v\n", err)

This comment has been minimized.

@alexellis

alexellis Mar 8, 2019

Member

Why did we drop the logging?

This comment has been minimized.

@acornies

acornies Mar 8, 2019

Author Member

it's only been moved.

This comment has been minimized.

@acornies

acornies Mar 8, 2019

Author Member

actually, going to update it leaving as is due to logging reason context.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 10, 2019

Is CI blocked? Do we know why?

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Mar 10, 2019

I don't see any link for CI details, which is really weird

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 10, 2019

Why is this PR draft? Is that why? Is this WIP or ready for review @acornies ?

case k8serrors.ReasonForError(err) == metav1.StatusReasonBadRequest:
log.Printf("Secret %s error reason: %s, %v\n", context, metav1.StatusReasonBadRequest, err)
return http.StatusBadRequest
case k8serrors.ReasonForError(err) == metav1.StatusReasonForbidden:

This comment has been minimized.

@alexellis

alexellis Mar 10, 2019

Member

I guess my concern with this approach is that we don't give the reason for the error back to the caller.

We should also be able to test this new method that has been broken out with unit tests when we have a final design on how it is going to work.

I am not super keen on logging in the function. I'd rather see you return the Status Code plus a message that we then chose to log in the calling function - thereby applying Single Responsibility Principle (SRP)

This comment has been minimized.

@acornies

acornies Mar 11, 2019

Author Member

Updated.

@LucasRoesler

This comment has been minimized.

Copy link
Member

LucasRoesler commented Mar 10, 2019

The last build on travis looks like this https://travis-ci.org/openfaas/faas-cli/builds/504025019

for 54ee31dd7e3d538783ba9466f81dc5ab98a793bd which does not match the current commit

@acornies

This comment has been minimized.

Copy link
Member Author

acornies commented Mar 10, 2019

Yeah, the draft PRs mess with CI usually. I chose it so that we could deliberate on the change first (pre-unit tests as well). I'll update with recent feedback.

Updates based on PR feedback:
- move logging back to caller
- return both http status code and k8s reason
- add tests for default and conflict reasons

Signed-off-by: Andrew Cornies <acornies@gmail.com>
@acornies

This comment has been minimized.

Copy link
Member Author

acornies commented Mar 11, 2019

I've updated the PR. Can someone flip this PR status to non-draft mode? I don't have permissions.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 11, 2019

I would have thought since it's your PR you can set it out of draft. What do the GH docs say? This is a new feature and I don't entirely see the point of it

@acornies

This comment has been minimized.

Copy link
Member Author

acornies commented Mar 11, 2019

Normally you can: https://help.github.com/en/articles/changing-the-stage-of-a-pull-request. Is it because I'm not a member of any teams?

@acornies acornies marked this pull request as ready for review Mar 12, 2019

@derek derek bot added the no-dco label Mar 12, 2019

@derek

This comment has been minimized.

Copy link

derek bot commented Mar 12, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

@acornies acornies force-pushed the acornies:error_reasons branch from 7639d21 to a4f434f Mar 12, 2019

@derek derek bot removed the no-dco label Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.