src: direct serialization of Function metadata as func.yaml

[lkingland]

Changes

🧹 direct serialization of Function metadata as func.yaml

This PR recognizes the logical equivalence of the Function and Config objects, merging the two and in so doing reduces some complexity, and frees up the concept of config for use by non-source-controlled function-local/user-local settings and transient state.

/kind cleanup

Details

The Config object was created after the initial Function draft. Its purpose was to enable manipulation of metadata about a Function. This separation between the Function and Config objects was partly to help keep the update minimally invasive, and possibly to draw a distinction between lifecycle functions such as "build" and "deploy" from metadata accessors such as "envs" and "labels". The Config object and its accessors have proved to be stable, well developed and useful. The distinction now needs to be removed.

The impetus for making this change now is that forthcoming features require the more idiomatic usage of the term "config". To summarize, "config" denotes aspects of the system which are configurable, but are not part of the Function itself. This includes things like installed repositories, user settings (preferred language/repository), build stamps/hashes, running function PIDs, etc. Taking the venerable git as an example: upon checking out a repository, the items configurable via the config subcommand are not included, but are rather populated based off local global presets or user-initiated action. They are either repo-local or user-local. They are not actually part of the repository insofar as being source controlled themselves. This is the purpose of "dotfiles". In the very-similar case of Functions, this currently includes Repositories, but is soon to be expanded to include metadata about settings such as preferred Function language, and state to enable func invoke, etc.

Looking from the top down, we can understand the necessity of this change by realizing that func.yaml is the serialized form of Function metadata, meaning that (when combined with source code), it is the Function itself. Support for this position is made clear by the existence of the following mapping:

func fromConfig(c Config) (f Function) {
	return Function{
		Name:            c.Name,
		Namespace:       c.Namespace,
		Runtime:         c.Runtime,
		Image:           c.Image,
		ImageDigest:     c.ImageDigest,
		Builder:         c.Builder,
		Builders:        c.Builders,
		Buildpacks:      c.Buildpacks,
		HealthEndpoints: c.HealthEndpoints,
		Volumes:         c.Volumes,
		BuildEnvs:       c.BuildEnvs,
		Envs:            c.Envs,
		Annotations:     c.Annotations,
		Options:         c.Options,
		Labels:          c.Labels,
	}
}

// toConfig serializes a Function to a config object.
func toConfig(f Function) Config {
	return Config{
		Name:            f.Name,
		Namespace:       f.Namespace,
		Runtime:         f.Runtime,
		Image:           f.Image,
		ImageDigest:     f.ImageDigest,
		Builder:         f.Builder,
		Builders:        f.Builders,
		Buildpacks:      f.Buildpacks,
		HealthEndpoints: f.HealthEndpoints,
		Volumes:         f.Volumes,
		BuildEnvs:       f.BuildEnvs,
		Envs:            f.Envs,
		Annotations:     f.Annotations,
		Options:         f.Options,
		Labels:          f.Labels,
	}
}

As can be seen above, since almost every field of a "Config" is mapped directly to a "Function" instance, it is a strong indication they are actually the same concept.

It is worth discussing a potential point in opposition to this change: it impies the need to flatten our command structure. Previously, at the root level were only lifecycle commands such as build and deploy. Let's call these "Verbs". Metadata-manipulation subcommands were held beneath config such as envs and labels . Let's call these "Nouns".

At first blush it may seem wrong or confusing to commingle these as root subcommands. The problem here, however, is not that this is linguistically incorrect, but rather that our chosen CLI library does not to my knowledge support grouping in the help text. Again taking 'git' as our example: at the root are both verbs such as git clone and git init, as well as nouns such as git branch and git tag. The distinction comes in the form of properly formatted help text, which groups these subcommands under appropriate headings (this can be seen by running the command git with no further arguments).

If the team agrees, we could emulate this user experience by providing a somewhat customized help text which groups our commands under appropriate headings as well. It will require a bit of customization, however, as it does not appear that the Cobra CLI library supports this directly. This change should also move the repositories subcommand in the opposite direction: from the root to be a subcommand of config, because these settings fit the description of being non-source-controled, local system configuration.

To enumerate the set of challenges which necessitate this change now:

1. The repositories management subcommand is not modifying the Function metadata, nor are its changes source controlled, and therefore should be func config repos .... However this is not currently possible while the config subcommand represents the Function's actual metadata.
2. The transient, local-only state of the hash of the container created on the last build should not be part of the function's serialized state, but it currently has nowhere to go except in func.yaml. Creating a config to encapsulate transient, local-only state like this is needed in order to avoid triggering an unstated git commit (modified func.yaml) when only building a function locally.
3. In addition to the transient build hash explained in pull list into client library #2, it is an upcoming need to track metadata to support the func invoke subcommand. This state is part of config, but not part of the function's serialized state, so it is dependent on this PRs change as well.
4. In order to provide a configurable default function language (a request identified during user interviews), it is necessary to have a git-like config with the ability to add a --global option, keeping the (again, local-only non-source-controlled configuration) in either ./.func/config.yaml or ~/.config/func/config.yaml.
5. The forthcoming on-cluster builds will require being able to configure settings for building a function (func config build ...) which are local-only, and not part of the source-controlled Function metadata. This is another example of a setting that should go in a config file, but not in func.yaml.

[knative-prow-robot]

@lkingland: The label(s) kind/cleanup cannot be applied, because the repository doesn't have them.

Details

In response to this:

Changes

🧹 Function directly serializes to func.yaml

This PR recognizes the logical equivalence of the Function and Config objects, merging the two and in so doing reduces some complexity, and frees up the concept of config for future use by non-source-controlled function-local and user-local settings.

/kind cleanup

Details

The Config object was created after the initial Function draft. It's purpose was to capture the need to manipulate metadata about the Function outside of the source code itself. This separation between the Function and Config objects was to keep the update minimally invasive, and possibly to draw a distinction between lifecycle functions such as "build" and "deploy" from metadata accessors such as "envs". Now that the Config object has proved to be stable, well developed and useful; the time has come to upgrade: to rectify the unnecessary distinction.

The impetus for making this change now is that forthcoming features require the more idiomatic usage of the term "config". To summarize, "config" denotes aspects of the system which are configurable, but are not part of the Function itself. This includes things like repositories, user settings, etc. Taking the venerable git as an example: upon checking out a repository, the items configurable via the config subcommand are not included, but are rather populated based off local global presets or user-initiated action. They are either repo-local or user-local. They are not actually part of the repository insofar as being source controlled themselves. This is the purpose of "dotfiles". In the very-similar case of Functions, this currently includes Repositories, but is soon to be expanded to include metadata about settings such as preferred Function language, and state to enable func invoke, etc.

Looking from the top down, we can understand the necessity of this change by realizing that func.yaml is the serialized form of Function metadata, meaning that (when combined with source code), it is the Function itself. Support for this position is made clear by the existence of the following mapping:

<< Code >>

As can be seen above, since almost every field of a "Config" is mapped directly to a "Function" instance, it is a strong indication they are actually the same concept.

It is worth discussing a potential point in opposition to this change: it impies the need to flatten our command structure. Previously, at the root level were only lifecycle commands such as build and deploy. Let's call these "Verbs". Metadata-manipulation subcommands were held beneath config such as envs and labels . Let's call these "Nouns".

At first blush it may seem wrong or confusing to commingle these as root subcommands. The problem here, however, is not that this is linguistically incorrect, but rather that our chosen CLI library does not to my knowledge support grouping in the help text. Again taking 'git' as our example: at the root are both verbs such as git clone and git init, as well as nouns such as git branch and git tag. The distinction comes in the form of properly formatted help text, which groups these subcommands under appropriate headings (this can be seen by running the command git with no further arguments).

If the team agrees, we could emulate this user experience by providing a somewhat customized help text which groups our commands under appropriate headings as well. It will require a bit of customization, however, as it does not appear that the Cobra CLI library supports this directly. This change should also move the repositories subcommand in the opposite direction: from the root to be a subcommand of config, because these settings fit the description of being non-source-controled, local system configuration.

To enumerate the set of challenges which necessitate this change now:

1. The repositories management subcommand is not modifying the Function metadata, nor are its changes source controlled, and therefore should be func config repos .... However this is not currently possible while the config subcommand represents the Function's actual metadata.
2. The transient, local-only state of the hash of the container created on the last build should not be part of the function's serialized state, but it currently has nowhere to go except in func.yaml. Creating a config to encapsulate transient, local-only state like this is needed in order to avoid triggering an unstated git commit (modified func.yaml) when only building a function locally.
3. In addition to the transient build hash explained in pull list into client library #2, it is an upcoming need to track metadata to support the func invoke subcommand. This state is part of config, but not part of the function's serialized state, so it is dependent on this PRs change as well.
4. In order to provide a configurable default function language (a request identified during user interviews), it is necessary to have a git-like config with the ability to add a --global option, keeping the (again, local-only non-source-controlled configuration) in either ./.func/config.yaml or ~/.config/func/config.yaml.
5. The forthcoming on-cluster builds will require being able to configure settings for building a function (func config build ...) which are local-only, and not part of the source-controlled Function metadata. This is another example of a setting that should go in a config file, but not in func.yaml.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [lkingland]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

Merging #641 (02b8b50) into main (852626a) will decrease coverage by 0.14%.
The diff coverage is 60.51%.

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
- Coverage   37.45%   37.31%   -0.15%     
==========================================
  Files          38       41       +3     
  Lines        3887     3859      -28     
==========================================
- Hits         1456     1440      -16     
+ Misses       2243     2229      -14     
- Partials      188      190       +2     

Impacted Files	Coverage Δ
cmd/build.go	27.52% <0.00%> (ø)
cmd/config.go	14.08% <0.00%> (ø)
cmd/config_envs.go	2.57% <0.00%> (ø)
cmd/config_volumes.go	4.91% <0.00%> (ø)
cmd/deploy.go	16.50% <0.00%> (ø)
cmd/run.go	40.38% <0.00%> (ø)
function_envs.go	31.37% <31.37%> (ø)
client.go	63.01% <33.33%> (+2.47%)	⬆️
function_volumes.go	50.00% <50.00%> (ø)
function.go	60.68% <62.68%> (+3.10%)	⬆️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 852626a...02b8b50. Read the comment docs.

[zroubalik]

/lgtm

[lkingland]

/lgtm

Thanks for the lgtm @zroubalik . I made a few changes since then, and would greatly appreciate a quick re-review if you have time,

[lance]

This is a big PR! I do very much like the fact that we no longer have both Config and Function. This is much cleaner.

I have to admit, I did not completely validate all of the copy/paste that went into some of the refactoring. I do like what you have done with, for example, the function environment variable handling now being in its own file. As long as all of the existing tests pass, I'll assume that all is good in that regard. There is only one place where it seems like a test has been commented out which has me wondering.

/lgtm
/hold to follow up on commented test and a few very very minor nits that don't really need to be held for but since we're holding anyway, I mentioned them

[lance]

/unhold
/lgtm