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.

Sign up for GitHub

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

Jump to bottom

Remove leaking logs from SDK #868

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

Remove leaking logs from SDK #868

wants to merge 1 commit into from

Conversation

viveksyngh
Copy link
Contributor

@viveksyngh viveksyngh commented Feb 23, 2021 •
edited

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 vivekkmr45@yahoo.in

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
alexellis
alexellis reviewed Feb 24, 2021
View reviewed changes
commands/deploy.go Outdated Show resolved Hide resolved
@viveksyngh viveksyngh requested a review from alexellis May 2, 2021 19:12
alexellis
alexellis reviewed Jun 9, 2021
View reviewed changes
proxy/errors.go
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
Remove leaking log from deploy
4f81de9 
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 <vivekkmr45@yahoo.in>
viveksyngh
viveksyngh commented Jun 26, 2021
View reviewed changes
proxy/deploy.go

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.

viveksyngh
viveksyngh commented Jun 26, 2021
View reviewed changes
commands/errors.go
@@ -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
Reviewers

@alexellis alexellis alexellis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove leaking logs from CLI proxy package
2 participants
@viveksyngh @alexellis