knative / func

Knative Functions client API and CLI
Apache License 2.0
275 stars 138 forks source link

Consider removing `describe` and `delete` & possibly `deploy` and `update` #126

Closed lance closed 4 years ago

lance commented 4 years ago

When we consider that this project's intent is to run as an inlined plugin in the kn CLI, the commands we use now as faas describe and faas delete become kn faas describe and kn faas delete. These are really no different than kn service describe and kn service delete. I don't think we should be duplicating this behavior that already exists in the target binary.

I think we should also discuss the implications of deploy and update. These commands are very similar to kn service create and kn service update, but actually do less. The deploy command does do add some annotations and labels to the deployment, so it may be necessary to keep. But certainly kn service update is hardly different than faas update.

/cc @boson-project/core

zroubalik commented 4 years ago

Agree on that.

Just a brain dump: What if kn faas describe is just a wrapper around kn service describe + printing some additional (functions related) info. This way we don't have to reimplement already existing functionality, but just adding the one we need. But I am not sure if this is something, that's even possible in kn.

lkingland commented 4 years ago

When we consider that this project's intent is to run as an inlined plugin in the kn CLI

If that is the intent, then yes these commands are redundant. However, if we would like the faas client library to provide a platform-agnostic set of Function manipulation primitives, which can be used across multiple deployment platforms, then perhaps these individual command should be retained in the faas binary and not "plumbed" through kn.

nainaz commented 4 years ago

This constant switch to kn faas to kn service could be very confusing and jarring. I believe consistency for Function Author will be paramount given they might not be from dev background. to create function I write kn faas init or kn faas create. but then to delete or describe I have to write kn service describe etc. Is there a way to wrap these kn commands in faas cli for better experience?

nainaz commented 4 years ago

I agree with the point @lkingland made here . However, if we would like the faas client library to provide a platform-agnostic set of Function manipulation primitives, which can be used across multiple deployment platforms. This should be our goal, platform agnostic in the long run.

lance commented 4 years ago

This should be our goal, platform agnostic in the long run.

Why? What other platform do we realistically expect this to be running on any time before 2022?

lance commented 4 years ago

Is there a way to wrap these kn commands in faas cli for better experience?

They already are in the faas CLI. I am proposing to remove them, because they are redundant. Certainly delete is, if not describe.

lkingland commented 4 years ago

I am not opposed to removing these if they are causing problems. However, perhaps we could leave them in the 'faas' binary, but just not plumb them through kn to avoid confusion. Here's my reasoning for having added them in the first place:

These are redundant if one's perspective is that the only purpose of the faas CLI is to be a plug-in for kn. However, we might have a very nice opportunity to provide a simple option for Function developers who are being supported by a cluster administrator.

The kn command, while full featured and rich, is a much higher learning curve than the simple Create, Read, Update, Delete provided by the faas CLI. With a properly configured cluster, these basic commands, plus perhaps List and Logs, might be enough to fully serve the needs of a beginning developer who is being supported by a cluster administrator wielding a powerful tool like kn.

It was my goal in creating these commands that we support the use cases needed by a Function developer who has access to a pre-configured cluster, without ever needing to install or use any other binary, while simultaneously creating a layer which is cluster agnostic. In particular the first additional platform being localhost

lance commented 4 years ago

See https://github.com/boson-project/faas/pull/128. I think I may close this issue.