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

Remove leaking logs from SDK #868

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

viveksyngh
Copy link
Contributor

@viveksyngh viveksyngh commented Feb 23, 2021

This commit removes leaking logs from the SDK (proxy package) of the CLI by removing
custom print statements within proxy package. It updates method signature for
DeployFunction which now returns deployOutput text along with status code.

Fixes: #853

Signed-off-by: Vivek Singh [email protected]

Description

Motivation and Context

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

How Has This Been Tested?

echo -n $PASSWORD | ./faas-cli login --username admin --password-stdin
Calling the OpenFaaS server to validate the credentials...
Handling connection for 8080
credentials saved for admin http://127.0.0.1:8080
(⎈ |kind-vdp:default)➜  faas-cli git:(remove-leakin-logs) echo -n $PASSWORD | ./faas-cli login --username admin --password-stdin
(⎈ |kind-vdp:default)➜  faas-cli git:(remove-leakin-logs) ./faas-cli store deploy figlet
Handling connection for 8080

Deployed. 202 Accepted.
URL: http://127.0.0.1:8080/function/figlet

(⎈ |kind-vdp:default)➜  faas-cli git:(remove-leakin-logs) ./faas-cli list
Handling connection for 8080
Function                      	Invocations    	Replicas
figlet                        	0              	1

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.

@viveksyngh viveksyngh marked this pull request as draft February 23, 2021 12:06
@viveksyngh viveksyngh marked this pull request as ready for review February 24, 2021 10:57
commands/deploy.go Outdated Show resolved Hide resolved
@viveksyngh viveksyngh requested a review from alexellis May 2, 2021 19:12
StatusReasonInternalError StatusReason = "InternalError"

// Status code 401
StatusReasonUnauthorized StatusReason = "Unauthorized"
Copy link
Member

Choose a reason for hiding this comment

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

Could it be http.StatusUnauthorized? https://golang.org/src/net/http/status.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can but we will have challenge in solving two problems, if we are not using StatusReason

  1. When we will have same http status code being returned for different errors. I think we are not doing that right now but will be helpful in future.

  2. Since we are only having typed errors for only handful of http status codes, so if API server returns an HTTP response code for which we don't have typed error, we are returning that as UnknownError embedding HTTP Response code and reason. We are checking for UnknownError based on HTTP response code. Please go through this file, https://github.com/openfaas/faas-cli/pull/868/files#diff-25e38af31c2b62e171bb1b4e9a70546d19aa8fbd180d435435814962e2912600

If we remove StatusReason, to figure out UnknownError we will have to check for list of all HTTP status for which we have typed error.

@viveksyngh viveksyngh force-pushed the remove-leakin-logs branch from a87570f to 0c9edf9 Compare June 26, 2021 10:07
This commit removing lekaing logs from deploy API in SDK. It also
returns typed errors from API resoponse and allows as to check for some
custom errors which functions like `IsNotFound` or `IsUnauthorized`.

This also updates DeployFuntion API to return a typed response and `http.Response`.

Signed-off-by: Vivek Singh <[email protected]>
@viveksyngh viveksyngh force-pushed the remove-leakin-logs branch from 0c9edf9 to 4f81de9 Compare June 26, 2021 10:13

if spec.Update == true && statusCode == http.StatusNotFound {
if spec.Update == true && IsNotFound(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of checking types error.

@@ -24,3 +27,13 @@ func checkTLSInsecure(gateway string, tlsInsecure bool) string {
}
return ""
}

//actionableErrorMessage print actionable error message based on APIError check
func actionableErrorMessage(err error) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A generic error message functions which can be used in all commands to print error message for generic APIError like 403, 500 etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove leaking logs from CLI proxy package
2 participants