openfaas / faas-swarm

OpenFaaS provider for Docker Swarm
https://github.com/openfaas/faas
MIT License
81 stars 37 forks source link

Replace proxy handler with provider implementation #39

Closed LucasRoesler closed 5 years ago

LucasRoesler commented 5 years ago

Description

Motivation and Context

How Has This Been Tested?

Additional unit tests have been provided to ensure that the resolver works as expected. The Proxy handler is unit tested upstream in the faas-provider

I have also manually tested this by deploying to my local docker swarm using the deploy_stack.sh in openfaas/faas project with two modifications

  1. update the docker-compose.yaml line 17 so that it has direct_functions: "false"
  2. update the docker-compose.yaml line 44 so that it has image: theaxer/faas-swarm:new-proxy

I then went to the UI on localhost, deployed a function from the Store and verified that the function invoke worked as expected. I then did the same form the CLI, building, deploying, and then invoking a function.

Types of changes

Note that this removes the x-function header functionality that allows the user to set the function name via a header value. This has been deprecated for some time and is not implemented in the faas-provider

Checklist:

alexellis commented 5 years ago

Hi Lucas, are there any functional changes in this PR?

Alex

LucasRoesler commented 5 years ago

This removes the deprecated xFuntionHeader functionality

xFunctionHeader := r.Header["X-Function"]
            if len(xFunctionHeader) > 0 {
                log.Print("X-Function: ", xFunctionHeader)
            }

Forgot about that change, I will update the notes

alexellis commented 5 years ago

Right the only valid values across the project are 0/1 and true/false in those cases.

LucasRoesler commented 5 years ago

@alexellis I have addressed each of your comments except for this one, which I left a comment on https://github.com/openfaas/faas-swarm/pull/39#discussion_r232453514 . Github incorrectly marked it as outdated due to another change near the same line.

LucasRoesler commented 5 years ago

@alexellis should be ready for a last look now

alexellis commented 5 years ago

Thanks so much @LucasRoesler, this was a big change across at least a couple of repos. I appreciate the attention to detail on this too.