fnproject / cli

CLI tool for fnproject.
https://fnproject.io
Apache License 2.0
131 stars 67 forks source link

What is the CLI for? Proposal: deprecate and remove `fn build` and `fn test`. #383

Open mjg123 opened 6 years ago

mjg123 commented 6 years ago

At the moment the CLI does two distinct things:

As far as "calls the API" goes, that's fine. Makes sense that we provide a tool for doing that. But the justification for "Builds images" is significantly weaker. We couldn't possibly make a better image building tool than docker itself, and we're spending a lot of time and effort trying to.

Proposal:

Work needed:

shaunsmith commented 6 years ago

I agree with the goal of extracting all language specific logic from the CLI and relying on the generation of an appropriate Dockerfile. See #369 for comments on init.

+1 on removal of fn test. I don't think this is being used.

Won't func.yaml runtime: always be docker and therefore unnecessary?

zootalures commented 6 years ago

‘runtime’ is only used for build I think (to generate the Dockerfile) so yes, becomes irrelevant

mjg123 commented 6 years ago

Tagging in @mantree @JadeRedworth @riconnon and @rdallman for their thoughts too.

tcoupland commented 6 years ago

This sounds like a great idea. Less is more, and what little there is should be tightly focused.

:+1:

denismakogon commented 6 years ago

I agree that fn build and fn test commands are quite useless.

However, if we gonna drop fn test we need to provide something else. FDK-Java started with their own integration with JUnit to let developers test their functions. I did the same for FDK-python: https://github.com/fnproject/fdk-python/pull/47

So, we need to do the following:

  1. Get rid of fn test, it's useless for something more complex than hello-world.
  2. Write a boilerplate for each FDK that would represent a test suite, here's an example of an extended boilerplate:
import fdk
import ujson

from fdk import fixtures

def handler(ctx, data=None, loop=None):
    name = "World"
    if data and len(data) > 0:
        body = ujson.loads(data)
        name = body.get("name")
    return {"message": "Hello {0}".format(name)}

class TestFuncWithData(fixtures.FunctionTestCase):
    content = ujson.dumps({"name": "John"})

    def setUp(self):
        super(TestFuncWithData, self).setUp(
            self.content, handler, fn_format="json")

    def tearDown(self):
        super(TestFuncWithData, self).tearDown()

    def test_response_with_data(self):
        def assert_data(data):
            return {"message": "Hello John"} == data

        self.assertResponseConsistent(
            assert_data,
            message="content must be equal to '{0}'"
            .format({"message": "Hello John"})
        )

    def test_in_time(self):
        self.assertInTime(1)

class TestFuncWithoutData(fixtures.FunctionTestCase):
    content = ""

    def setUp(self):
        super(TestFuncWithoutData, self).setUp(
            self.content, handler, fn_format="cloudevent")

    def tearDown(self):
        super(TestFuncWithoutData, self).tearDown()

    def test_response_without_data(self):

        def assert_data(data):
            return {"message": "Hello World"} == data

        self.assertResponseConsistent(
            assert_data,
            message="content must be equal to '{0}'"
            .format({"message": "Hello World"})
        )

    def test_in_time(self):
        self.assertInTime(1)

if __name__ == "__main__":
    fdk.handle(handler)

as you can see i've added few test suites that actually can be executed with native pytest tool. We MUST to do the same for every officially supported FDK.

  1. Provide a way to run these tests inside of a container (similar to fn test but with help of native tools instead of what we have now).
mjg123 commented 6 years ago

NB: format is currently controlled by LangHelper - they all use json except Java/Kotlin which uses http.

NB2: The following CLI flags would be removed: cmd, entrypoint, format

rdallman commented 6 years ago

fn test am not sure of its fate, but I'm not sure if the scope is right here. In theory it seems useful to be able to test a container contract for things like changing the prog language used for a function to something else, but I'm not sure if anybody is using that functionality at all. It is a DSL in its own way, maybe killing it is the 'right' thing to do.

fn init generating a Dockerfile seems nice, I've on numerous occasions wanted this because I need to make some tiny tweak to the Dockerfile that gets made-but-hidden and the way we have it now is very magical. I wonder what the % of users is on this and whether we should still generate at build/deploy time without spitting out the Dockerfile for users that don't want to see it / modify it (i.e. it's some optional thing like fn build --dry spits out a Dockerfile).

just thinking out loud ^

mjg123 commented 6 years ago

I agree - maybe I've conflated too many things into one issue here? The main win for me here would be if every function had a Dockerfile and was runtime: docker. Well, then we wouldn't need the runtime specified at all, and the CLI wouldn't need to have any runtime-specific code. And, at that point fn build would be the same as docker build wouldn't it? I'm very open to having fn test remain - or at least to stopping talking about it on this issue...

rdallman commented 6 years ago

yeah, it would be nice if func.yaml was just configuration for a function/trigger rather than some weird unclear mix of local build fields and remote configuration fields. generating the Dockerfile in init gives you that instead of pushing to build step, and init only runs once too. fwiw there was a push once upon a time to mask all the docker stuff from users that don't want to deal with it and that's how we got to where we are, now we've come full circle :) I like the idea of the dockerfile in init pretty well, there's some wrench to be thrown about other oci compatible runtimes but maybe punt that whole convo

shaunsmith commented 6 years ago

I got the latest CLI build with the new init --init-image support and was disappointed that the output didn't include a Dockerfile. 😞 @mjg123 I'm +1 for your original proposal and so far other than some question of the usefulness of fn test I think all other commentator are too. So let's move ahead on this and split out the fn test issue for separate discussion.