src(commands): remove --path flag from most commands #125

lance · 2020-09-16T19:48:11Z

This commit removes the --path -p flag for the build, delete, deploy,
describe, run and update commands. The init and create commands have been
left alone for now, pending the outcome of #123.
The list command doesn't need to know a path, because it's about seeing
all deployed Functions.

Adds a faas.LoadFunction(path string) function to load an existing Function
project from disk and return an error if one does not already exist.

Note: I realize this might be controversial. Speak up if you disagree with this proposal!

Edit: The idea here is to enforce the notion that all commands (other than init and create) are run from within a Function project directory where the configuration file .faas.yaml defines the context. Some of the comments from @matejvasek identified areas where functionality has been reduced - e.g. you can't describe or delete a function by name. I think we should actually consider removing these, and possibly even consider removing faas deploy and faas update.

BREAKING CHANGE

This commit removes the `--path -p` flag for the build, delete, deploy, describe, run and update commands. The init and create commands have been left alone for now, pending the outcome of knative#123. The list command doesn't need to know a path, because it's about seeing all deployed Functions. Adds a `faas.LoadFunction(path string)` function to load an existing Function project from disk and return an error if one does not already exist.

matejvasek · 2020-09-17T15:06:07Z

cmd/delete.go

 	deleteCmd.Flags().StringP("namespace", "n", "", "Override namespace in which to search for Functions.  Default is to use currently active underlying platform setting - $FAAS_NAMESPACE")
 }

 var deleteCmd = &cobra.Command{
-	Use:               "delete <name>",
+	Use:               "delete",


Why not allow to delete function by name?

Conceptually, the idea that this PR puts forward is that without a --path flag, all commands must be run from within a project directory where for all commands, there is a configuration file .faas.yaml that describes the context, and the commands run within this context.

Related... the delete command almost seems unnecessary to me, since this is already possible with kn, and our ultimate intent is to embed these commands inline in our kn distro. I considered removing it entirely, considering kn service delete <service-name> does exactly the same thing as kn faas delete.

With both of these scenarios in mind, today @rhuss and I discussed the potential for the project configuration file to move into upstream. If that happens, then it would be kn service delete and kn faas delete doing exactly the same thing and that really seems unnecessary.

kn service delete and kn faas delete doing exactly the same thing

Yes they practically are same thing.

I discussed the potential for the project configuration file to move into upstream.

Not sure what this mean, what conf file, what upstream?

Not sure what this mean, what conf file, what upstream?

The .faas.yaml file, moving to something like .kn.yaml in knative/client to generically describe a Knative project.

matejvasek · 2020-09-17T15:07:39Z

cmd/describe.go

@@ -28,18 +27,26 @@ func init() {
 }

 var describeCmd = &cobra.Command{
-	Use:               "describe <name> [options]",
+	Use:               "describe [options]",


Why shouldn't be I able to ask for description of function by name (which is not in cwd)?

Basically for the same reasons I wrote in my comment above about delete. I think describe is another candidate for removal because how is kn service describe <service-name> any worse than kn faas describe? And again, if config moves upstream, then kn service describe === kn faas describe.

kn service describe === kn faas describe

Not exactly same, faas has slightly different formatting and it also pulls info about subscriptions/triggers which kn doesn't IIRC, am I right @rhuss?

kn has separate sub-command for subscriptions/triggers.

Not exactly same, faas has slightly different formatting and it also pulls info about subscriptions/triggers which kn doesn't

Fair point. Let's discuss all of these commands in #126

zroubalik

LGTM

lkingland · 2020-09-18T12:40:07Z

If there is a compelling reason to remove this flag, I am all for agreeing, but I beleve there is a compelling reason it was added in the first place, which I will present below:

The reason for the path flag is primarily to support users who would like to script their usage of the faas binary without having to change directories to the target directory before running commands. For instance, if one must change directory before running the command, a script can no longer be a simple single command, but now must have state: it must couple the previous command (cd ...) with the following command (faas ...). The purpose is very much similar to the --directory flag of the make command, which I have personally found quite helpful when scripting.

The idea of the --path flag is to run the command as if the command had been run from within that directory. I believe this to be a valid and quite helpful use case.

Furthermore, by providing this flag as a configuration parameter, we ensure that this variable is explicitly provided in the client API. By removing it we run the risk of the client library thinking it can sample the current working directory willy-nilly, which could cause problems when used as a library in multi-threaded applications which legitimately change directories during operation.

So If this flag is only being removed to enforce that the client user is running the command from within the directory, I actually would have to disagree. Commands should take their effective directory as an explicit input, which can be defaulted to the current working directory.

I hope that adequately presents the case for the existence of this flag, but am happy to capitulate with but a modicum of pushback!

lkingland · 2020-09-18T13:01:12Z

function.go

+// OverrideImage overwrites (or sets) the value of the Function's .Image
+// property, which preempts the default functionality of deriving the value as:
+// Deafult:  [config.Repository]/[config.Name]:latest
+func (f Function) OverrideImage(image string) error {
+	if image == "" {
+		return nil
+	}
+	f.Image = image
+	return f.WriteConfig()
+}


FYI, in casd you are wondering why this function was in the CLI codebase not the "core" logic: it was because this overriding is specifically an override of that image prior to the invocation of the client. In away mutating the state of the function prior to actually invoking the client. This is a way to provide the ability to override the image name via a flag without "muddying up" the client library such that it can instead always trust what is in .faas.yaml.

lance · 2020-09-18T15:27:39Z

Marking as a draft for now and may rework some of this. @zroubalik will stalling this affect #127?

zroubalik · 2020-09-18T17:46:54Z

@lance I could finish #127 without any changes wrt to --path and the related stuff or I can wait for the resolution on the overall CLI UX decision. I'd prefer the latter, as we can implement that properly

lance · 2020-09-18T19:06:25Z

@zroubalik check out #128. I think we can use that to iterate on what the CLI behavior should be, with concrete descriptions, etc. I've written it to reflect how the CLI behaves today with the exception of init and create. Those I wrote to reflect the work in your PR here.

lance added kind/enhancement info/discussion source/commands labels Sep 16, 2020

lance requested a review from a team September 16, 2020 19:48

lance self-assigned this Sep 16, 2020

lance added 2 commits September 16, 2020 15:49

fixup: remove leftover functionWithOverrides

718847b

fixup: check return value of Override function calls

02bf97a

matejvasek reviewed Sep 17, 2020

View reviewed changes

zroubalik mentioned this pull request Sep 18, 2020

feat: decouple function name from function domain #127

Merged

zroubalik approved these changes Sep 18, 2020

View reviewed changes

matejvasek approved these changes Sep 18, 2020

View reviewed changes

lkingland reviewed Sep 18, 2020

View reviewed changes

lance marked this pull request as draft September 18, 2020 15:26

lance mentioned this pull request Sep 18, 2020

docs: add documentation for CLI commands #128

Merged

lance closed this Sep 21, 2020

lance mentioned this pull request Sep 23, 2020

src: add Long command descriptions for each of the CLI commands #133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src(commands): remove --path flag from most commands #125

src(commands): remove --path flag from most commands #125

lance commented Sep 16, 2020 •

edited

matejvasek Sep 17, 2020

lance Sep 17, 2020

matejvasek Sep 17, 2020

matejvasek Sep 17, 2020

lance Sep 17, 2020

matejvasek Sep 17, 2020

lance Sep 17, 2020

matejvasek Sep 17, 2020 •

edited

matejvasek Sep 17, 2020

lance Sep 17, 2020

zroubalik left a comment

lkingland commented Sep 18, 2020

lkingland Sep 18, 2020

lance commented Sep 18, 2020

zroubalik commented Sep 18, 2020

lance commented Sep 18, 2020

src(commands): remove --path flag from most commands #125

src(commands): remove --path flag from most commands #125

Conversation

lance commented Sep 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matejvasek Sep 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

lkingland commented Sep 18, 2020

Choose a reason for hiding this comment

lance commented Sep 18, 2020

zroubalik commented Sep 18, 2020

lance commented Sep 18, 2020

lance commented Sep 16, 2020 •

edited

matejvasek Sep 17, 2020 •

edited