cli icon indicating copy to clipboard operation
cli copied to clipboard

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

Open mjg123 opened this issue 6 years ago • 10 comments

At the moment the CLI does two distinct things:

  • builds images
  • calls the API in order to deploy functions

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:

  • fn init always generates a Dockerfile
  • fn build becomes a synonym for docker build (we don't need to actually remove it I guess)
  • fn test removed
  • func.yaml is purely a deployment descriptor. The following keys which relate to building images on the host are deprecated and removed:
    • build, build_image, cmd, content_type, entrypoint, expects, format, headers, run_image, runtime, tests

Work needed:

  • init-images: #369
  • reduction in number of hard-coded runtimes: #380
  • supported init-images should produce dockerfiles and runtime: docker in the boilerplate
  • remove all LangHelper code from CLI
  • deprecate build-related keys from func.yaml
  • remove all build code from CLI

mjg123 avatar Aug 14 '18 09:08 mjg123

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?

shaunsmith avatar Aug 15 '18 06:08 shaunsmith

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

zootalures avatar Aug 15 '18 07:08 zootalures

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

mjg123 avatar Aug 16 '18 10:08 mjg123

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

:+1:

tcoupland avatar Aug 16 '18 13:08 tcoupland

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).

denismakogon avatar Aug 16 '18 13:08 denismakogon

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

mjg123 avatar Aug 17 '18 13:08 mjg123

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 ^

rdallman avatar Aug 20 '18 23:08 rdallman

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...

mjg123 avatar Aug 21 '18 18:08 mjg123

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

rdallman avatar Aug 21 '18 18:08 rdallman

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.

shaunsmith avatar Aug 22 '18 16:08 shaunsmith