Skip to content
This repository has been archived by the owner on Mar 10, 2023. It is now read-only.

Vendor SDK for function-logs #620

Merged

Conversation

martindekov
Copy link
Contributor

@martindekov martindekov commented Mar 15, 2020

Vendor faas-cli SDK for function logs
instead of creating custom http requests.
Also rewriting unit tests and removing
excess error handling which is moved in
the SDK and is excessive as we don't
parse JSON into the function anymore

Signed-off-by: Martin Dekov mvdekov@gmail.com

Description

Part of #609

How Has This Been Tested?

On my own private instance with image containing the change martindekov/function-logs:0.3.0-rc3:

functions/function-logs:0.0.2 commit log output:

https://github.com/martindekov/push2/runs/509201597
image

martindekov/function-logs:0.3.0-rc3 commit log output:

https://github.com/martindekov/push2/runs/509210079
image

UI's Build Logs with same image:

How are existing users impacted? What migration steps/scripts do we need?

N/A Internal change

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s
  • added unit tests
  • augmented unit tests

@alexellis
Copy link
Member

Just a comment on the wrapping of the commit message. What width are you using as a template? It seems too narrow.

@alexellis
Copy link
Member

@martindekov FYI I think the testing of the build logs may be irrelevant here since they are not using the OpenFaaS REST API, they are simply the output of the of-builder service. Can you confirm if that's your understanding too?

function-logs/handler.go Outdated Show resolved Hide resolved
@martindekov
Copy link
Contributor Author

I will need to re-visit this change as per your comment.

@alexellis
Copy link
Member

Thanks Martin

@martindekov martindekov force-pushed the martindekov/vendor-function-logs branch from b77b36c to a0ccde9 Compare March 21, 2020 15:20
Copy link
Contributor

@viveksyngh viveksyngh left a comment

Choose a reason for hiding this comment

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

I have left a comment, other changes looks fine to me.

function-logs/handler.go Outdated Show resolved Hide resolved
Vendor faas-cli SDK for function logs
instead of creating custom http requests.
Also rewriting unit tests and removing
excess error handling which is moved in
the SDK and is excessive as we don't
parse JSON into the function anymore

Signed-off-by: Martin Dekov <mvdekov@gmail.com>
@martindekov martindekov force-pushed the martindekov/vendor-function-logs branch from a0ccde9 to 5fded2f Compare March 29, 2020 10:54
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Approved, given the testing and approval from @viveksyngh

@alexellis alexellis merged commit d22cdd3 into openfaas:master Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants