From a57fb91ddf44890eb2450f037a772696ab23b852 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 23 Sep 2026 10:26:53 +0200 Subject: [PATCH 1/7] bug: namespace in confirm mode is not used --- cmd/deploy.go | 5 ++++- cmd/deploy_test.go | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 28451e1a51..027ceecbf2 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -579,7 +579,10 @@ type deployConfig struct { // clusters). If not provided, the currently configured namespace will be // used. For instance, that which would be used by default by `kubectl` // (~/.kube/config) in the case of Kubernetes. - Namespace string + // The survey tag routes the namespace prompt's answer here: by name alone + // survey would pick the embedded config.Global.Namespace, which Configure + // records as the namespace the function is already deployed in. + Namespace string `survey:"namespace"` //Service account to be used in deployed function ServiceAccountName string diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 2e636d4289..b374f297c3 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/AlecAivazis/survey/v2/core" "github.com/ory/viper" "github.com/spf13/cobra" @@ -1100,6 +1101,28 @@ func TestDeploy_Namespace(t *testing.T) { } } +// TestDeploy_NamespacePromptAnswer ensures the answer to the namespace prompt +// (--confirm) becomes the namespace to deploy to. deployConfig shadows the +// Namespace of its embedded config.Global, and survey matched that one by +// name: the function was deployed to the default namespace and, deployed +// locally, a function of the same name in the chosen namespace was removed. +func TestDeploy_NamespacePromptAnswer(t *testing.T) { + cfg := deployConfig{Namespace: "default"} + if err := core.WriteAnswer(&cfg, "namespace", "chosen"); err != nil { + t.Fatal(err) + } + f, err := cfg.Configure(fn.Function{Name: "f", Runtime: "go", Root: t.TempDir()}) + if err != nil { + t.Fatal(err) + } + if f.Namespace != "chosen" { + t.Errorf("expected the answered namespace to be deployed to, got %q", f.Namespace) + } + if f.Deploy.Namespace != "" { + t.Errorf("expected no namespace recorded as deployed, got %q", f.Deploy.Namespace) + } +} + // TestDeploy_NamespaceDefaultsToK8sContext ensures that when not specified, a // users's active kubernetes context is used for the namespace if available. func TestDeploy_NamespaceDefaultsToK8sContext(t *testing.T) { From 283a8847ccf59d5ed03d949488ba33f449151bb8 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Fri, 4 Sep 2026 09:31:02 +0200 Subject: [PATCH 2/7] fix: the pipeline builds and labels the commit a git function was read at The image's revision label came from the local checkout's HEAD for every remote build. For a function read from its repository that is the wrong commit, or none. Source gains an in-memory Commit, the hash the revision resolved to when the function was read; when set, the PipelineRun fetches that commit and labels the image with it, so what was read is what is built. Uploaded sources keep the local HEAD. The PipelineRun no longer turns an empty revision into a hard-coded "main": the revision goes to the fetch step as configured, and an empty one fetches the remote's default branch, whatever its name. --- pkg/functions/function_source.go | 24 ++++++++ pkg/functions/function_source_unit_test.go | 16 ++++++ pkg/pipelines/tekton/templates.go | 36 +++++++++--- pkg/pipelines/tekton/templates_pack.go | 2 +- pkg/pipelines/tekton/templates_s2i.go | 2 +- pkg/pipelines/tekton/templates_test.go | 67 ++++++++++++++++++++++ 6 files changed, 137 insertions(+), 10 deletions(-) diff --git a/pkg/functions/function_source.go b/pkg/functions/function_source.go index c4f2576eda..6e0a10d9d8 100644 --- a/pkg/functions/function_source.go +++ b/pkg/functions/function_source.go @@ -18,6 +18,13 @@ type Source struct { // Dir is the directory within the repository holding the function. // Empty means the repository root. Dir string `yaml:"dir,omitempty"` + + // Commit is the full hash Revision resolved to when the function was read + // from its repository, in memory only, never stored. The cluster then + // fetches and labels the image with exactly this commit. Empty for a local + // or uploaded source, which the builders label from the git working tree + // on disk: its HEAD, marked dirty when there are uncommitted changes. + Commit string `yaml:"-" json:"-"` } // validateSource validates the source option from Function config @@ -37,5 +44,22 @@ func validateSource(source Source) (errors []string) { errors = append(errors, errMsg) } } + if source.Commit != "" && !isFullHash(source.Commit) { + errors = append(errors, fmt.Sprintf("source commit %q is not a full commit hash", source.Commit)) + } return } + +// isFullHash reports whether s is a full commit hash: 40 lowercase +// hexadecimal digits, as git and go-git print one. +func isFullHash(s string) bool { + if len(s) != 40 { + return false + } + for _, c := range s { + if !strings.ContainsRune("0123456789abcdef", c) { + return false + } + } + return true +} diff --git a/pkg/functions/function_source_unit_test.go b/pkg/functions/function_source_unit_test.go index 93be5217b5..e73c5ad2d6 100644 --- a/pkg/functions/function_source_unit_test.go +++ b/pkg/functions/function_source_unit_test.go @@ -78,6 +78,22 @@ func Test_validateSource(t *testing.T) { Source{}, 0, }, + { + "correct 'Source - URL + full commit", + Source{ + URL: "https://myrepo/foo.git", + Commit: "0123456789abcdef0123456789abcdef01234567", + }, + 0, + }, + { + "incorrect 'Source - abbreviated commit", + Source{ + URL: "https://myrepo/foo.git", + Commit: "0123456", + }, + 1, + }, } for _, tt := range tests { diff --git a/pkg/pipelines/tekton/templates.go b/pkg/pipelines/tekton/templates.go index 362ce1a057..0650ef0dce 100644 --- a/pkg/pipelines/tekton/templates.go +++ b/pkg/pipelines/tekton/templates.go @@ -347,6 +347,31 @@ func createAndApplyPipelineTemplate(f fn.Function, namespace string, labels map[ return createAndApplyResource(f.Root, pipelineFileName, template, "pipeline", getPipelineName(f), namespace, data) } +// sourceRevision returns what the cluster fetches and what it labels the +// image with. A function read from its repository has the commit it was +// read at, so the cluster fetches exactly that. Anything else is fetched by +// the revision as configured, the remote's default branch when empty, and +// labelled from the git working tree on disk if it has one, HEAD marked +// dirty when there are uncommitted changes, as every builder labels a local +// source. +func sourceRevision(f fn.Function) (fetch, label string) { + if c := f.Build.Source.Commit; c != "" { + // validateSource requires a full hash; the guard spares a caller that + // skipped validation a panic. + label = c + if len(label) > 7 { + label = label[:7] + } + return c, label + } + // Only a function on disk has a working tree to label from: with no + // Root, GitCommit would search whichever repository func runs in. + if f.Root != "" { + label, _ = fn.GitCommit(f.Root) + } + return f.Build.Source.Revision, label +} + // createAndApplyPipelineRunTemplate creates and applies PipelineRun template for a standard on-cluster build // all resources are created on the fly, if there's a PipelineRun defined in the project directory, it is used instead func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels map[string]string) error { @@ -357,11 +382,6 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m contextDir = "." } - pipelinesTargetBranch := f.Build.Source.Revision - if pipelinesTargetBranch == "" { - pipelinesTargetBranch = defaultPipelinesTargetBranch - } - buildEnvs := []string{} if len(f.Build.BuildEnvs) == 0 { buildEnvs = []string{"="} @@ -389,7 +409,7 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m tlsVerify = "false" } - commit, _ := fn.GitCommit(f.Root) + fetch, label := sourceRevision(f) data := templateData{ FunctionName: f.Name, @@ -408,10 +428,10 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m S2iImageScriptsUrl: s2iImageScriptsUrl, TlsVerify: tlsVerify, - Commit: commit, + Commit: label, RepoUrl: f.Build.Source.URL, - Revision: pipelinesTargetBranch, + Revision: fetch, } var template string diff --git a/pkg/pipelines/tekton/templates_pack.go b/pkg/pipelines/tekton/templates_pack.go index 25a53f804d..b2c7d2824e 100644 --- a/pkg/pipelines/tekton/templates_pack.go +++ b/pkg/pipelines/tekton/templates_pack.go @@ -107,7 +107,7 @@ spec: - name: gitRepository value: "{{.RepoUrl}}" - name: gitRevision - value: {{.Revision}} + value: "{{.Revision}}" - name: contextDir value: "{{.ContextDir}}" - name: imageName diff --git a/pkg/pipelines/tekton/templates_s2i.go b/pkg/pipelines/tekton/templates_s2i.go index 8eb5ccd7a0..7834f2a0da 100644 --- a/pkg/pipelines/tekton/templates_s2i.go +++ b/pkg/pipelines/tekton/templates_s2i.go @@ -114,7 +114,7 @@ spec: - name: gitRepository value: "{{.RepoUrl}}" - name: gitRevision - value: {{.Revision}} + value: "{{.Revision}}" - name: contextDir value: "{{.ContextDir}}" - name: imageName diff --git a/pkg/pipelines/tekton/templates_test.go b/pkg/pipelines/tekton/templates_test.go index 7fc519d184..b09e0fa52d 100644 --- a/pkg/pipelines/tekton/templates_test.go +++ b/pkg/pipelines/tekton/templates_test.go @@ -7,7 +7,10 @@ import ( "strings" "testing" "text/template" + "time" + gogit "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" "github.com/manifestival/manifestival" "github.com/manifestival/manifestival/fake" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" @@ -346,6 +349,70 @@ func Test_createAndApplyPipelineRunTemplate(t *testing.T) { } } +// Test_sourceRevision ensures a function read from its repository makes the +// cluster fetch, and label the image with, the commit it was read at, while +// any other function keeps the configured revision and the local HEAD. +func Test_sourceRevision(t *testing.T) { + const hash = "0123456789abcdef0123456789abcdef01234567" + + f := fn.Function{Build: fn.BuildSpec{Source: fn.Source{URL: "https://example.com/repo.git", Revision: "main", Commit: hash}}} + if fetch, label := sourceRevision(f); fetch != hash || label != hash[:7] { + t.Errorf("read from git: expected %q, %q; got %q, %q", hash, hash[:7], fetch, label) + } + + f = fn.Function{Build: fn.BuildSpec{Source: fn.Source{URL: "https://example.com/repo.git", Revision: "v1"}}} + if fetch, _ := sourceRevision(f); fetch != "v1" { + t.Errorf("git without a resolved commit: expected the revision as configured, got %q", fetch) + } + + f = fn.Function{Root: t.TempDir()} // no git history, no repository + if fetch, label := sourceRevision(f); fetch != "" || label != "" { + t.Errorf("upload without history: expected \"\", \"\"; got %q, %q", fetch, label) + } + + // A commit shorter than the label (validation rejects it; a caller may + // skip validation) must not panic. + f = fn.Function{Build: fn.BuildSpec{Source: fn.Source{URL: "https://example.com/repo.git", Commit: "abc"}}} + if fetch, label := sourceRevision(f); fetch != "abc" || label != "abc" { + t.Errorf("short commit: expected \"abc\", \"abc\"; got %q, %q", fetch, label) + } + + // With no Root there is no working tree to label from, even when func + // runs inside some unrelated repository. + t.Chdir(gitRepoWithCommit(t)) + f = fn.Function{Build: fn.BuildSpec{Source: fn.Source{URL: "https://example.com/repo.git", Revision: "v1"}}} + if fetch, label := sourceRevision(f); fetch != "v1" || label != "" { + t.Errorf("rootless without a commit: expected \"v1\", \"\"; got %q, %q", fetch, label) + } +} + +// gitRepoWithCommit returns a new directory holding a git repository with +// one commit. +func gitRepoWithCommit(t *testing.T) string { + t.Helper() + dir := t.TempDir() + repo, err := gogit.PlainInit(dir, false) + if err != nil { + t.Fatal(err) + } + wt, err := repo.Worktree() + if err != nil { + t.Fatal(err) + } + if err = os.WriteFile(filepath.Join(dir, "README.md"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if _, err = wt.Add("README.md"); err != nil { + t.Fatal(err) + } + if _, err = wt.Commit("initial", &gogit.CommitOptions{ + Author: &object.Signature{Name: "test", Email: "test@example.com", When: time.Now()}, + }); err != nil { + t.Fatal(err) + } + return dir +} + // strictTektonDecoder returns a strict deserializer that rejects unknown fields // in Tekton v1 resources (Pipeline, PipelineRun, etc.). func strictTektonDecoder(t *testing.T) runtime.Decoder { From ae832ba1d4b19cdb866dd4272b6e6b52b118c34d Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 2 Sep 2026 20:49:56 +0200 Subject: [PATCH 3/7] feat: load a function from a git repository without a checkout NewFunctionFromGit reads func.yaml from a revision of a remote repository (default branch, branch, tag or commit hash) in memory, and records the commit that revision resolved to in Build.Source.Commit, for the build to fetch and label. It reuses the credential lookup the template repositories already use. Migrations used to re-read the on-disk func.yaml from f.Root to see the previous structure, so a function with no working tree could not be migrated. hasInitializedFunction in the client hit the same problem by migrating an unmarshalled function that had no Root. Migrations now receive the serialized bytes they were parsed from, and NewFunction, NewFunctionFromGit and hasInitializedFunction share parseFunction. The build prompt takes the function instead of reading it from a path, so it works for a function that is not on disk. It no longer asks for the project path: every other default it offers already came from the function at the original path, and a changed answer was reloaded by build and run but silently ignored by deploy and config git set. The path is given with --path, as before, and each command reads the function once. Groundwork for knative/func#3203. --- cmd/build.go | 29 +-- cmd/config_git_set.go | 2 +- cmd/deploy.go | 126 ++++++----- cmd/deploy_test.go | 208 ++++++++++++++++-- cmd/run.go | 12 +- docs/reference/func_deploy.md | 4 + e2e/e2e_remote_test.go | 55 +---- pkg/functions/client.go | 8 +- pkg/functions/function.go | 60 ++++-- pkg/functions/function_git.go | 238 +++++++++++++++++++++ pkg/functions/function_git_test.go | 188 ++++++++++++++++ pkg/functions/function_migrations.go | 127 +++++------ pkg/pipelines/tekton/pipelines_provider.go | 6 + pkg/pipelines/tekton/templates.go | 5 +- pkg/pipelines/tekton/templates_test.go | 29 +++ pkg/testing/testing.go | 60 ++++++ 16 files changed, 913 insertions(+), 244 deletions(-) create mode 100644 pkg/functions/function_git.go create mode 100644 pkg/functions/function_git_test.go diff --git a/cmd/build.go b/cmd/build.go index f7f4e25822..245d9923be 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -153,15 +153,16 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro cfg buildConfig f fn.Function ) - if cfg, err = newBuildConfig().Prompt(); err != nil { + cfg = newBuildConfig() + if f, err = fn.NewFunction(cfg.Path); err != nil { // Read in the Function + return + } + if cfg, err = cfg.Prompt(f); err != nil { return wrapPromptError(err, "build") } if err = cfg.Validate(cmd); err != nil { // Perform any pre-validation return wrapValidateError(err, "build") } - if f, err = fn.NewFunction(cfg.Path); err != nil { // Read in the Function - return - } if !f.Initialized() { return NewErrNotInitializedFromPath(f.Root, "build") } @@ -300,8 +301,9 @@ func (c buildConfig) Configure(f fn.Function) fn.Function { // Prompt the user with value of config members, allowing for interactive changes. // Skipped if not in an interactive terminal (non-TTY), or if --confirm false (agree to -// all prompts) was set (default). -func (c buildConfig) Prompt() (buildConfig, error) { +// all prompts) was set (default). f is the function being configured, which +// need not be on the local filesystem. +func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { // If there is no registry nor explicit image name defined, the // Registry prompt is shown whether or not we are in confirm mode. // Otherwise, it is only shown if in confirm mode @@ -309,10 +311,6 @@ func (c buildConfig) Prompt() (buildConfig, error) { // value and will always use the value from the config (flag or env variable). // This is not strictly correct and will be fixed when Global Config: Function // Context is available (PR#1416) - f, err := fn.NewFunction(c.Path) - if err != nil { - return c, err - } // Check if function exists first if !f.Initialized() { @@ -330,7 +328,7 @@ func (c buildConfig) Prompt() (buildConfig, error) { err := survey.AskOne( &survey.Input{Message: "Registry for function images:", Default: c.Registry}, &c.Registry, - survey.WithValidator(NewRegistryValidator(c.Path))) + survey.WithValidator(NewRegistryValidator(f))) if err != nil { return c, fn.ErrRegistryRequired } @@ -353,13 +351,6 @@ func (c buildConfig) Prompt() (buildConfig, error) { Message: "Optionally specify an exact image name to use (e.g. quay.io/boson/node-sample:latest)", }, }, - { - Name: "path", - Prompt: &survey.Input{ - Message: "Project path:", - Default: c.Path, - }, - }, { Name: "builder", Prompt: &survey.Select{ @@ -377,7 +368,7 @@ func (c buildConfig) Prompt() (buildConfig, error) { }, } - err = survey.Ask(qs, &c) + err := survey.Ask(qs, &c) if err != nil { return c, err } diff --git a/cmd/config_git_set.go b/cmd/config_git_set.go index 77368853e4..f6a64e89bc 100644 --- a/cmd/config_git_set.go +++ b/cmd/config_git_set.go @@ -155,7 +155,7 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) { func (c configGitSetConfig) Prompt(f fn.Function) (configGitSetConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { return c, err } diff --git a/cmd/deploy.go b/cmd/deploy.go index 027ceecbf2..16783ce130 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "errors" "fmt" "io" @@ -13,7 +14,6 @@ import ( "github.com/spf13/cobra" "k8s.io/apimachinery/pkg/api/resource" "knative.dev/client/pkg/util" - "knative.dev/func/cmd/common" "knative.dev/func/pkg/builders" "knative.dev/func/pkg/config" "knative.dev/func/pkg/deployers" @@ -81,6 +81,10 @@ DESCRIPTION eliminating the need for a local container engine. To trigger deployment of a git repository instead of local source, combine with '--source': '{{rootCmdUse}} deploy --remote --source=git.example.com/alice/f.git' + A branch, tag or commit is given with '--revision': + '{{rootCmdUse}} deploy --remote --source=git.example.com/alice/f.git --revision=v1.2.0' + The function is then read from the repository, so no local copy is + needed. Choose the directory within the repository with '--source-dir'. Domain When deploying, a function's route is automatically generated using the @@ -261,34 +265,25 @@ EXAMPLES func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { var ( - cfg deployConfig - f fn.Function + cfg deployConfig + f fn.Function // the function to deploy + local fn.Function // the function at cfg.Path, if any ) // Initialize config first cfg = newDeployConfig(cmd) - // Create function object to check if initialized - if f, err = fn.NewFunction(cfg.Path); err != nil { + // Load the function at path. It is the function to deploy unless the + // source is a git repository, in which case it only records the outcome. + if local, err = fn.NewFunction(cfg.Path); err != nil { return } - - // Check if function exists BEFORE prompting for config - if !f.Initialized() { - if !cfg.Remote || f.Build.Source.URL == "" { - // Only error if this is not a fully remote build - return NewErrNotInitializedFromPath(f.Root, "deploy") - } else { - // TODO: this case is not supported because the pipeline - // implementation requires the function's name, which is in the - // remote repository. We should inspect the remote repository. - // For now, give a more helpful error. - return errors.New("please ensure the function's source is also available locally") - } + if f, err = cfg.function(cmd.Context(), local); err != nil { + return } // Now that we know function exists, proceed with prompting - if cfg, err = cfg.Prompt(); err != nil { + if cfg, err = cfg.Prompt(f); err != nil { if errors.Is(err, fn.ErrRegistryRequired) { return NewErrRegistryRequired(err, "deploy") } @@ -297,6 +292,12 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { if err = cfg.Validate(cmd); err != nil { return wrapValidateError(err, "deploy") } + // The prompt may have made the source a git repository + if cfg.Remote && cfg.Source != "" && f.Root != "" { + if f, err = cfg.function(cmd.Context(), local); err != nil { + return + } + } // Warn if registry changed but registryInsecure is still true warnRegistryInsecureChange(cmd.OutOrStderr(), cfg.Registry, f) @@ -439,6 +440,24 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { } // Write + // A function deployed from a git repository has no working tree of its own. + // A local function at path, if there is one, records the request and the + // outcome so that later commands (describe, delete, another deploy) find + // them; its own metadata is left alone. + if f.Root == "" { + if !local.Initialized() { + return nil + } + if local, err = cfg.Configure(local); err != nil { + return + } + local.Registry = f.Registry + local.Deploy.Image = f.Deploy.Image + local.Deploy.Namespace = f.Deploy.Namespace + local.Deploy.Deployer = f.Deploy.Deployer + local.Deploy.Expose = f.Deploy.Expose + f = local + } if err = f.Write(); err != nil { return } @@ -450,6 +469,32 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return f.Stamp() } +// function returns the function to deploy. When a git repository is the +// source of a remote deployment it is the function committed there: the +// pipeline must describe what the cluster builds, and a local checkout may +// be absent, on another branch or in another directory. Otherwise it is the +// given local function, which must be initialized. +func (c deployConfig) function(ctx context.Context, local fn.Function) (fn.Function, error) { + if c.Remote && c.Source != "" { + return fn.NewFunctionFromGit(ctx, c.gitSource()) + } + if !local.Initialized() { + return local, NewErrNotInitializedFromPath(local.Root, "deploy") + } + return local, nil +} + +// gitSource is the git repository to build from, as configured: the URL +// may carry the revision as a fragment (#), which then +// wins, as it does in Configure. +func (c deployConfig) gitSource() fn.Source { + g := fn.Source{URL: c.Source, Revision: c.Revision, Dir: c.SourceDir} + if parts := strings.SplitN(c.Source, "#", 2); len(parts) == 2 { + g.URL, g.Revision = parts[0], parts[1] + } + return g +} + // build determines if the function should be built based on given flag func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { if flag == "auto" { @@ -469,7 +514,7 @@ func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { return false, nil } -func NewRegistryValidator(path string) survey.Validator { +func NewRegistryValidator(f fn.Function) survey.Validator { return func(val interface{}) error { // if the value passed in is the zero value of the appropriate type @@ -477,15 +522,10 @@ func NewRegistryValidator(path string) survey.Validator { return fn.ErrRegistryRequired } - f, err := fn.NewFunction(path) - if err != nil { - return err - } - // Set the function's registry to that provided f.Registry = val.(string) - _, err = f.ImageName() //image can be derived without any error + _, err := f.ImageName() //image can be derived without any error if err != nil { return fmt.Errorf("invalid registry [%q]: %w", val.(string), err) } @@ -667,9 +707,9 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { // Configure basic members f.Domain = c.Domain f.Namespace = c.Namespace - f.Build.Source.URL = c.Source - f.Build.Source.Dir = c.SourceDir - f.Build.Source.Revision = c.Revision + commit := f.Build.Source.Commit // the commit a function read from git was read at + f.Build.Source = c.gitSource() + f.Build.Source.Commit = commit f.Build.RemoteStorageClass = c.RemoteStorageClass f.Deploy.ServiceAccountName = c.ServiceAccountName f.Deploy.ImagePullSecret = c.ImagePullSecret @@ -694,15 +734,6 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { if err != nil { return f, err } - - // .Revision - // TODO: the system should support specifying revision (refSpec) as a URL - // fragment ([#]) throughout, which, when implemented, removes - // the need for the below split into separate members: - if parts := strings.SplitN(c.Source, "#", 2); len(parts) == 2 { - f.Build.Source.URL = parts[0] - f.Build.Source.Revision = parts[1] - } return f, nil } @@ -723,9 +754,9 @@ func applyEnvs(current []fn.Env, args []string) (final []fn.Env, err error) { // Prompt the user with value of config members, allowing for interactive changes. // Skipped if not in an interactive terminal (non-TTY), or if --yes (agree to // all prompts) was explicitly set. -func (c deployConfig) Prompt() (deployConfig, error) { +func (c deployConfig) Prompt(f fn.Function) (deployConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { return c, err } @@ -933,21 +964,6 @@ func printDeployMessages(out io.Writer, f fn.Function) { if !f.Local.Remote && (f.Build.Source.URL != "" || f.Build.Source.Revision != "" || f.Build.Source.Dir != "") { fmt.Fprintf(out, "Warning: source settings are only applicable when running with --remote. Local source code will be used.") } - - // Git Branch Mismatch - // ------------------- - // When doing a remote build with --revision, warn if the local branch - // doesn't match, as this can lead to confusion about which func.yaml is used. - if f.Local.Remote && f.Build.Source.URL != "" && f.Build.Source.Revision != "" { - // Doing a remote build, specified a git repository to pull from, and - // specified a reference within that remote. - currentBranch, err := common.DefaultCurrentBranch(f.Root) - if err != nil { - fmt.Fprintf(out, "Warning: unable to verify local and remote references match. %v\n", err) - } else if currentBranch != f.Build.Source.Revision { - fmt.Fprintf(out, "Warning: Local git branch '%s' does not match --revision '%s'. The local func.yaml will be used for function metadata (name, runtime, etc). Ensure your local branch matches the remote branch to avoid deployment issues.\n", currentBranch, f.Build.Source.Revision) - } - } } // isDigested checks that the given image reference has a digest. Invalid diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index b374f297c3..2b2f978529 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -534,16 +534,162 @@ func testFunctionContext(cmdFn commandConstructor, t *testing.T) { } } +// funcYAML is the content of a func.yaml for a go function of the given +// name, with extra appended as further top-level keys. +func funcYAML(name, extra string) string { + return "specVersion: " + fn.LastSpecVersion() + "\nname: " + name + + "\nruntime: go\ncreated: 2024-01-01T00:00:00Z\n" + extra +} + +// serveFunction serves a git repository holding, on main, a func.yaml for +// a go function of the given name, and returns the repository's URL. +func serveFunction(t *testing.T, name string) string { + t.Helper() + url, _ := ServeGitRepository(t, map[string]map[string]string{ + "main": {"func.yaml": funcYAML(name, "")}, + }) + return url +} + +// TestDeploy_RemoteGitNoLocalFunction ensures a remote deployment of a git +// repository needs no local copy of the function: the function is read from +// the repository, and nothing is written to the (empty) current directory. +// +// func deploy --remote --source={url} +// +// https://github.com/knative/func/issues/3203 +func TestDeploy_RemoteGitNoLocalFunction(t *testing.T) { + root := FromTempDirectory(t) + // Only the requested branch and directory hold a function named + // remote-fn, so deploying it shows both were honoured. + url, heads := ServeGitRepository(t, map[string]map[string]string{ + "main": {"func.yaml": funcYAML("root-fn", "")}, + "feature": {"functions/remote-fn/func.yaml": funcYAML("remote-fn", "")}, + }) + + pipeliner := mock.NewPipelinesProvider() + var deployed fn.Function + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + deployed = f + return base(f) + } + + cmd := NewDeployCmd(NewTestClient( + fn.WithPipelinesProvider(pipeliner), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--remote", + "--source=" + url, + "--revision=feature", + "--source-dir=functions/remote-fn", + "--namespace=fnns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + // The pipeline received the function from the repository, configured + // by the flags, and builds the commit it was read at + want := fn.Source{URL: url, Revision: "feature", Dir: "functions/remote-fn", Commit: heads["feature"]} + if !pipeliner.RunInvoked { + t.Fatal("pipeline was not invoked") + } + if deployed.Name != "remote-fn" { + t.Errorf("expected the repository's function to be deployed, got %q", deployed.Name) + } + if deployed.Root != "" { + t.Errorf("expected no root, got %q", deployed.Root) + } + if deployed.Namespace != "fnns" || deployed.Build.Source != want { + t.Errorf("expected flags to configure the deployed function, got %+v", deployed) + } + // Nothing was written locally + if _, err := os.Stat(filepath.Join(root, fn.FunctionFile)); !os.IsNotExist(err) { + t.Errorf("expected no %s to be written, got err %v", fn.FunctionFile, err) + } +} + +// TestDeploy_RemoteGitUsesRepositoryFunction ensures that, when a local +// function exists alongside a remote deployment of a git repository, the +// pipeline is created for the function as committed in the repository, +// while the local function records the request and the outcome. +func TestDeploy_RemoteGitUsesRepositoryFunction(t *testing.T) { + root := FromTempDirectory(t) + url := serveFunction(t, "remote-fn") + + if _, err := fn.New().Init(fn.Function{Name: "local-fn", Runtime: "node", Root: root}); err != nil { + t.Fatal(err) + } + + pipeliner := mock.NewPipelinesProvider() + var deployed fn.Function // as returned by the pipeline: the outcome + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + url, f, err := base(f) + deployed = f + return url, f, err + } + cmd := NewDeployCmd(NewTestClient( + fn.WithPipelinesProvider(pipeliner), + fn.WithRegistry(TestRegistry), + )) + cmd.SetArgs([]string{"--remote", "--source=" + url, "--namespace=fnns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + if deployed.Name != "remote-fn" || deployed.Runtime != "go" { + t.Errorf("expected the repository's function to be deployed, got %q (%v)", deployed.Name, deployed.Runtime) + } + + local, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) + } + if local.Name != "local-fn" || local.Runtime != "node" { + t.Errorf("expected the local function's metadata to be untouched, got %q (%v)", local.Name, local.Runtime) + } + if local.Build.Source.URL != url { + t.Errorf("expected the git source to be recorded locally, got %q", local.Build.Source.URL) + } + if local.Deploy.Namespace != "fnns" { + t.Errorf("expected the deployed namespace to be recorded locally, got %q", local.Deploy.Namespace) + } + if local.Deploy.Image != deployed.Deploy.Image || local.Deploy.Image == "" { + t.Errorf("expected the deployed image %q to be recorded locally, got %q", deployed.Deploy.Image, local.Deploy.Image) + } +} + +// TestDeploy_RemoteGitLoadError ensures a repository the function cannot be +// read from fails the deployment before any pipeline is run. +func TestDeploy_RemoteGitLoadError(t *testing.T) { + _ = FromTempDirectory(t) + url := serveFunction(t, "remote-fn") + + pipeliner := mock.NewPipelinesProvider() + cmd := NewDeployCmd(NewTestClient(fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry))) + cmd.SetArgs([]string{"--remote", "--source=" + url, "--revision=nope"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "not found") { + t.Fatalf("expected the loader's error, got %v", err) + } + if pipeliner.RunInvoked { + t.Error("pipeline should not run when the function cannot be read") + } +} + // TestDeploy_RemoteSourcePersists ensures that the source flags, if provided, // are persisted to the Function for subsequent deployments. func TestDeploy_RemoteSourcePersists(t *testing.T) { root := FromTempDirectory(t) var ( - url = "https://example.com/user/repo" branch = "main" dir = "function" ) + url, _ := ServeGitRepository(t, map[string]map[string]string{ + "main": {"function/func.yaml": funcYAML("repo-fn", "")}, + }) // Create a new Function in the temp directory f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) @@ -580,8 +726,11 @@ func TestDeploy_RemoteSourceEnv(t *testing.T) { if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) } - t.Setenv("FUNC_SOURCE", "https://example.com/user/repo") - t.Setenv("FUNC_REVISION", "v1.2.0") + url, _ := ServeGitRepository(t, map[string]map[string]string{ + "release": {"function/func.yaml": funcYAML("repo-fn", "")}, + }) + t.Setenv("FUNC_SOURCE", url) + t.Setenv("FUNC_REVISION", "release") t.Setenv("FUNC_SOURCE_DIR", "function") cmd := NewDeployCmd(NewTestClient( @@ -597,22 +746,29 @@ func TestDeploy_RemoteSourceEnv(t *testing.T) { if err != nil { t.Fatal(err) } - want := fn.Source{URL: "https://example.com/user/repo", Revision: "v1.2.0", Dir: "function"} + want := fn.Source{URL: url, Revision: "release", Dir: "function"} if f.Build.Source != want { t.Errorf("expected source %+v from the environment, got %+v", want, f.Build.Source) } } // TestDeploy_RemoteSourceUsed ensures that any source values provided as flags are used -// when invoking a remote deployment. +// when invoking a remote deployment: to read the function from the repository +// and as the pipeline's source. func TestDeploy_RemoteSourceUsed(t *testing.T) { root := FromTempDirectory(t) var ( - url = "https://example.com/user/repo" branch = "main" dir = "function" ) + // Only the requested directory holds a function named repo-fn + url, _ := ServeGitRepository(t, map[string]map[string]string{ + "main": { + "func.yaml": funcYAML("root-fn", ""), + "function/func.yaml": funcYAML("repo-fn", ""), + }, + }) // Create a new Function in the temp dir _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) if err != nil { @@ -622,6 +778,9 @@ func TestDeploy_RemoteSourceUsed(t *testing.T) { // A Pipelines Provider which will validate the expected values were received pipeliner := mock.NewPipelinesProvider() pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + if f.Name != "repo-fn" { + t.Errorf("expected the function read from %s at %s, got %q", dir, branch, f.Name) + } if f.Build.Source.URL != url { t.Errorf("Pipeline Provider expected git URL '%v' got '%v'", url, f.Build.Source.URL) } @@ -645,6 +804,9 @@ func TestDeploy_RemoteSourceUsed(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatal(err) } + if !pipeliner.RunInvoked { + t.Fatal("pipeline was not invoked") + } } // TestDeploy_RemoteSourceFragment ensures that a --source which specifies the branch @@ -657,15 +819,26 @@ func TestDeploy_RemoteSourceFragment(t *testing.T) { t.Fatal(err) } + // Only the branch named in the fragment holds a function named repo-fn + expectedUrl, _ := ServeGitRepository(t, map[string]map[string]string{ + "main": {"func.yaml": funcYAML("root-fn", "")}, + "branch": {"func.yaml": funcYAML("repo-fn", "")}, + }) var ( - url = "https://example.com/user/repo#branch" - expectedUrl = "https://example.com/user/repo" + url = expectedUrl + "#branch" expectedBranch = "branch" ) + pipeliner := mock.NewPipelinesProvider() + var deployed fn.Function + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + deployed = f + return base(f) + } cmd := NewDeployCmd(NewTestClient( fn.WithDeployer(mock.NewDeployer()), fn.WithBuilder(mock.NewBuilder()), - fn.WithPipelinesProvider(mock.NewPipelinesProvider()), + fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry), )) cmd.SetArgs([]string{"--remote", "--source=" + url}) @@ -673,6 +846,9 @@ func TestDeploy_RemoteSourceFragment(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatal(err) } + if deployed.Name != "repo-fn" { + t.Errorf("expected the function to be read from %q at %q, got %q", expectedUrl, expectedBranch, deployed.Name) + } f, err = fn.NewFunction(root) if err != nil { @@ -1646,7 +1822,7 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) { var ( remoteValues = []string{"", "true", "false"} buildValues = []string{"", "true", "false", "auto"} - urlValues = []string{"", "https://example.com/user/repo"} + urlValues = []string{"", serveFunction(t, "repo-fn")} // toArgs converts one permutaton of the values into command arguments toArgs = func(remote, build, url string) []string { @@ -1860,8 +2036,9 @@ func TestDeploy_UnsetFlag(t *testing.T) { } // Deploy it, specifying a Git URL + url := serveFunction(t, "f") cmd := NewDeployCmd(NewTestClient()) - cmd.SetArgs([]string{"--remote", "--source=https://git.example.com/alice/f"}) + cmd.SetArgs([]string{"--remote", "--source=" + url}) if err := cmd.Execute(); err != nil { t.Fatal(err) } @@ -1871,7 +2048,7 @@ func TestDeploy_UnsetFlag(t *testing.T) { if err != nil { t.Fatal(err) } - if f.Build.Source.URL != "https://git.example.com/alice/f" { + if f.Build.Source.URL != url { t.Fatalf("url not persisted") } @@ -3051,6 +3228,7 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { root := FromTempDirectory(t) cleanup := k8s.SetOpenShiftForTest(true, nil) defer cleanup() + url := serveFunction(t, "repo-fn") if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) @@ -3064,9 +3242,9 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { base := pipeliner.RunFn pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { // add exposure tracking to the base RunFn - url, f, err := base(f) + endpoint, f, err := base(f) f.Deploy.Expose = tt.observed - return url, f, err + return endpoint, f, err } cmd := NewDeployCmd(NewTestClient( @@ -3077,7 +3255,7 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { cmd.SetOut(&out) cmd.SetErr(&out) cmd.SetArgs([]string{"--remote", - "--source=https://example.com/user/repo", + "--source=" + url, "--deployer=raw", "--expose=route"}) if err := cmd.Execute(); err != nil { t.Fatal(err) diff --git a/cmd/run.go b/cmd/run.go index f60ef3e9aa..c8ac5ee8ac 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -154,13 +154,13 @@ func runRun(cmd *cobra.Command, newClient ClientFactory) (err error) { cfg runConfig f fn.Function ) - if cfg, err = newRunConfig(cmd).Prompt(); err != nil { - return wrapPromptError(err, "run") - } - + cfg = newRunConfig(cmd) if f, err = fn.NewFunction(cfg.Path); err != nil { return } + if cfg, err = cfg.Prompt(f); err != nil { + return wrapPromptError(err, "run") + } if !f.Initialized() { return NewErrNotInitializedFromPath(f.Root, "run") } @@ -371,10 +371,10 @@ func (c runConfig) Configure(f fn.Function) (fn.Function, error) { return f, err } -func (c runConfig) Prompt() (runConfig, error) { +func (c runConfig) Prompt(f fn.Function) (runConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { return c, err } diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index 3e3013629b..4fc166c678 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -57,6 +57,10 @@ DESCRIPTION eliminating the need for a local container engine. To trigger deployment of a git repository instead of local source, combine with '--source': 'func deploy --remote --source=git.example.com/alice/f.git' + A branch, tag or commit is given with '--revision': + 'func deploy --remote --source=git.example.com/alice/f.git --revision=v1.2.0' + The function is then read from the repository, so no local copy is + needed. Choose the directory within the repository with '--source-dir'. Domain When deploying, a function's route is automatically generated using the diff --git a/e2e/e2e_remote_test.go b/e2e/e2e_remote_test.go index e122ee91d2..45467bd19c 100644 --- a/e2e/e2e_remote_test.go +++ b/e2e/e2e_remote_test.go @@ -5,7 +5,6 @@ package e2e import ( "fmt" "os" - "os/exec" "path/filepath" "testing" "time" @@ -42,21 +41,15 @@ func TestRemote_Deploy(t *testing.T) { } // TestRemote_Source ensures a remote build can be triggered which pulls -// source from a remote repository. +// source from a remote repository, with no local copy of the function. // // func deploy --remote --source={url} --registry={} --builder=pack func TestRemote_Source(t *testing.T) { name := "func-e2e-test-remote-source" _ = fromCleanEnv(t, name) - // This command currently requires the function source also be available - // locally in order to use its name. - cmd := exec.Command("git", "clone", "https://github.com/functions-dev/func-e2e-tests", ".") - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - - // Trigger the deploy + // Trigger the deploy from an empty directory: the function is read from + // the repository. if err := newCmd(t, "deploy", "--remote", "--source", "https://github.com/functions-dev/func-e2e-tests", "--registry", Registry, @@ -77,28 +70,12 @@ func TestRemote_Source(t *testing.T) { // TestRemote_Ref ensures a remote build can be triggered which pulls // source from a specific reference (branch/tag) of a remote repository. +// The function's metadata (name, runtime, etc) is read from that reference, +// so no local checkout is involved. func TestRemote_Ref(t *testing.T) { name := "func-e2e-test-remote-ref" _ = fromCleanEnv(t, name) - // This command currently requires the function source also be available - // locally in order to use its name. - cmd := exec.Command("git", "clone", "https://github.com/functions-dev/func-e2e-tests", ".") - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - - // IMPORTANT: The local func.yaml must match the one in the target branch. - // This is a current limitation where remote builds still require local - // source to determine function metadata (name, runtime, etc). - // TODO: Remove this checkout once the implementation supports fetching - // function metadata from the remote repository. - // https://github.com/knative/func/issues/3203 - cmd = exec.Command("git", "checkout", name) - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - // Trigger the deploy if err := newCmd(t, "deploy", "--remote", "--source", "https://github.com/functions-dev/func-e2e-tests", @@ -120,32 +97,14 @@ func TestRemote_Ref(t *testing.T) { } // TestRemote_Dir ensures that remote builds can be instructed to build and -// deploy a function located in a subdirectory. +// deploy a function located in a subdirectory of the repository. The +// function's metadata is read from that subdirectory. // -// func deploy --remote --source-dir={subdir} // func deploy --remote --source-dir={subdir} --source={url} func TestRemote_Dir(t *testing.T) { name := "func-e2e-test-remote-dir" _ = fromCleanEnv(t, name) - // This command currently requires the function source also be available - // locally in order to use its name. - cmd := exec.Command("git", "clone", "https://github.com/functions-dev/func-e2e-tests", ".") - if err := cmd.Run(); err != nil { - t.Fatal(err) - } - - // IMPORTANT: When using --source-dir, we need to change to that directory locally - // to ensure the local func.yaml matches the one that will be used in the remote build. - // This is a current limitation where remote builds still require local source to - // determine function metadata (name, runtime, etc). - // TODO: Remove this cd once the implementation supports fetching function metadata - // from the remote repository subdirectory. - // https://github.com/knative/func/issues/3203 - if err := os.Chdir(name); err != nil { - t.Fatalf("failed to change to subdirectory %s: %v", name, err) - } - // Trigger the deploy if err := newCmd(t, "deploy", "--remote", "--source", "https://github.com/functions-dev/func-e2e-tests", diff --git a/pkg/functions/client.go b/pkg/functions/client.go index c926956fae..18fef0f40b 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -18,7 +18,6 @@ import ( "time" "golang.org/x/sync/errgroup" - "gopkg.in/yaml.v2" "knative.dev/func/pkg/deployers" "knative.dev/func/pkg/utils" ) @@ -1609,11 +1608,8 @@ func hasInitializedFunction(path string) (bool, error) { if err != nil { return false, err } - f := Function{} - if err = yaml.Unmarshal(bb, &f); err != nil { - return false, err - } - if f, err = f.Migrate(); err != nil { + f, err := parseFunction(bb) + if err != nil { return false, err } return f.Initialized(), nil diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 1e6906fbb9..8d4a46e8b4 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -437,24 +437,10 @@ func NewFunction(root string) (f Function, err error) { if err != nil { return } - var functionMarshallingError error - var functionMigrationError error - if marshallingErr := yaml.Unmarshal(bb, &f); marshallingErr != nil { - functionMarshallingError = formatUnmarshalError(marshallingErr) // human-friendly unmarshalling errors - } - if f, err = f.Migrate(); err != nil { - functionMigrationError = err - } - // Only if migration fail return errors to the user. include marshalling error if present - if functionMigrationError != nil { - //returning both migrations and marshalling errors to the user - errorText := "Error: \n" - if functionMarshallingError != nil { - errorText += "Marshalling: " + functionMarshallingError.Error() - } - errorText += "\n" + "Migration: " + functionMigrationError.Error() - return Function{}, errors.New(errorText) + if f, err = parseFunction(bb); err != nil { + return } + f.Root = root f.Local, err = f.newLocal() if err != nil { @@ -467,13 +453,37 @@ func NewFunction(root string) (f Function, err error) { return } -// Validate function is logically correct, returning a bundled, and quite -// verbose, formatted error detailing any issues. -func (f Function) Validate() error { - if f.Root == "" { - return errors.New("function root path is required") +// parseFunction unmarshals a serialized function (the content of a func.yaml) +// and migrates it to the current spec version. The result has no Root: where +// the bytes came from is the caller's concern. +// +// Unmarshalling errors are reported only when the migration also fails. A +// function whose migration succeeds is accepted even if some of its fields +// did not unmarshal cleanly. +func parseFunction(bb []byte) (f Function, err error) { + f.Build.BuilderImages = make(map[string]string) + f.Deploy.Annotations = make(map[string]string) + var marshallingErr error + if err = yaml.Unmarshal(bb, &f); err != nil { + marshallingErr = formatUnmarshalError(err) // human-friendly unmarshalling errors + } + if f, err = f.migrate(bb); err != nil { + // Return both the migration and any marshalling error to the user + errorText := "Error: \n" + if marshallingErr != nil { + errorText += "Marshalling: " + marshallingErr.Error() + } + errorText += "\n" + "Migration: " + err.Error() + return Function{}, errors.New(errorText) } + return f, nil +} +// Validate function is logically correct, returning a bundled, and quite +// verbose, formatted error detailing any issues. Where the function lives is +// not part of its correctness: a function read from a git repository has no +// Root, which Write requires. +func (f Function) Validate() error { var ctr int errs := [][]string{ validateVolumes(f.Run.Volumes), @@ -596,6 +606,9 @@ func (f Function) MarshalFuncYaml() ([]byte, error) { // Write Function struct (metadata) to Disk at f.Root func (f Function) Write() (err error) { + if f.Root == "" { + return ErrRootRequired + } // Skip writing (and dirtying the work tree) if there were no modifications. f1, _ := NewFunction(f.Root) if reflect.DeepEqual(f, f1) { @@ -743,7 +756,8 @@ func (f Function) HasScaffolding() bool { // https://github.com/knative/func/pull/3436) and can interfere with other // builders (mostly just pack). func WarnIfLegacyS2IScaffolding(f Function, w io.Writer) { - if !f.HasScaffolding() { + // A function without a Root has no working tree to inspect. + if !f.HasScaffolding() || f.Root == "" { return } legacyAssemble := filepath.Join(f.Root, ".s2i", "bin", "assemble") diff --git a/pkg/functions/function_git.go b/pkg/functions/function_git.go new file mode 100644 index 0000000000..ffa4926d8d --- /dev/null +++ b/pkg/functions/function_git.go @@ -0,0 +1,238 @@ +package functions + +import ( + "context" + "errors" + "fmt" + "os" + "path" + "strings" + + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/memfs" + "github.com/go-git/go-billy/v5/util" + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/transport" + "github.com/go-git/go-git/v5/storage/memory" +) + +// NewFunctionFromGit loads the function committed in the repository g +// describes: the func.yaml in g.Dir at g.Revision, which is the remote's +// default branch when empty. A revision may be a branch, a tag or a full +// commit hash. +// +// No working copy is involved. The returned function therefore has no Root +// and none of the state NewFunction reads from one (local settings, last +// built image). Its Build.Source is g, where it was read from, with the +// commit g's revision resolved to: whatever source settings the committed +// func.yaml holds are replaced, so the commit is never paired with another +// repository. +func NewFunctionFromGit(ctx context.Context, g Source) (Function, error) { + if errs := validateSource(g); len(errs) > 0 { + return Function{}, errors.New(strings.Join(errs, "; ")) + } + src, err := resolveGitSource(ctx, g) + if err != nil { + return Function{}, err + } + tree, commit, err := src.checkout(ctx) + if err != nil { + return Function{}, err + } + bb, err := util.ReadFile(tree, path.Join(g.Dir, FunctionFile)) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return Function{}, fmt.Errorf("no %s in %q of %s at %s", FunctionFile, g.Dir, g.URL, src.describe()) + } + return Function{}, fmt.Errorf("cannot read %s from %s: %w", FunctionFile, g.URL, err) + } + f, err := parseFunction(bb) + if err != nil { + return Function{}, err + } + // Where the function was read from, and the commit it was read at, for + // the build to fetch and label. + f.Build.Source = g + f.Build.Source.Commit = commit.String() + return f, nil +} + +// gitSource is a revision of a remote repository, in the form the fetch +// needs it: a ref to clone, a commit to fetch, or neither for the remote's +// default branch. +type gitSource struct { + url string + auth transport.AuthMethod + // ref is the branch or tag to clone. Empty for the default branch, and + // for a bare commit, which hash then names. + ref plumbing.ReferenceName + hash plumbing.Hash +} + +// resolveGitSource decides how g is fetched. An empty revision is the +// remote's default branch, a full ref name (refs/...) and a full commit hash +// are taken as given; none of these needs the remote's refs. A bare name does: +// it is matched against them as git matches one (see gitrevisions), a tag +// before a branch, so a tag wins over a branch of the same name as it does +// for git fetch. Listing the refs costs one round trip and, on large +// repositories, a sizeable advertisement, hence only for bare names. +func resolveGitSource(ctx context.Context, g Source) (gitSource, error) { + src := gitSource{url: g.URL} + if g.URL == "" { + return src, errors.New("git URL required") + } + switch { + case g.Revision == "": + return src, nil + case strings.HasPrefix(g.Revision, "refs/"): + src.ref = plumbing.ReferenceName(g.Revision) + return src, nil + case plumbing.IsHash(g.Revision): + src.hash = plumbing.NewHash(g.Revision) + return src, nil + } + + remote := git.NewRemote(memory.NewStorage(), &config.RemoteConfig{ + Name: git.DefaultRemoteName, + URLs: []string{g.URL}, + }) + var refs []*plumbing.Reference + err := withAuth(g.URL, &src.auth, func(auth transport.AuthMethod) (err error) { + refs, err = remote.ListContext(ctx, &git.ListOptions{Auth: auth}) + return + }) + if err != nil { + return src, fmt.Errorf("cannot list refs of %s: %w", g.URL, err) + } + byName := make(map[plumbing.ReferenceName]bool, len(refs)) + for _, r := range refs { + byName[r.Name()] = true + } + for _, name := range []plumbing.ReferenceName{ + plumbing.NewTagReferenceName(g.Revision), + plumbing.NewBranchReferenceName(g.Revision), + } { + if byName[name] { + src.ref = name + return src, nil + } + } + if isAbbreviatedHash(g.Revision) { + // A remote cannot resolve an abbreviation: it serves objects by their + // full id, and neither can the cluster's fetch. + return src, fmt.Errorf("revision %q not found in %s: a commit must be given as its full hash", g.Revision, g.URL) + } + return src, fmt.Errorf("revision %q not found in %s", g.Revision, g.URL) +} + +// withAuth runs op without credentials and, if the remote asked for +// authentication, once more with those the local git configuration holds +// for url. The credentials that worked are left in auth for later calls. +func withAuth(url string, auth *transport.AuthMethod, op func(transport.AuthMethod) error) error { + err := op(*auth) + if isAuthError(err) && *auth == nil { + if a := credentialsForURL(url); a != nil { + *auth = a + err = op(a) + } + } + return err +} + +// isAbbreviatedHash reports whether s looks like a shortened commit hash. +func isAbbreviatedHash(s string) bool { + if len(s) < 4 || len(s) >= 40 { + return false + } + for _, c := range s { + if !strings.ContainsRune("0123456789abcdef", c) { + return false + } + } + return true +} + +// describe returns the revision for messages: the ref's short name, the +// commit hash, or HEAD for the remote's default branch. +func (s gitSource) describe() string { + switch { + case s.ref != "": + return s.ref.Short() + case !s.hash.IsZero(): + return s.hash.String() + } + return "HEAD" +} + +// checkout fetches the resolved revision, depth one, into memory and returns +// its tree and the commit it is. +func (s *gitSource) checkout(ctx context.Context) (billy.Filesystem, plumbing.Hash, error) { + var repo *git.Repository + err := withAuth(s.url, &s.auth, func(auth transport.AuthMethod) (err error) { + if !s.hash.IsZero() { + // A bare commit cannot be cloned: fetch it by hash and check it out. + repo, err = fetchGitCommit(ctx, s.url, s.hash, auth) + return + } + // An empty ReferenceName clones the remote's default branch. + repo, err = git.CloneContext(ctx, memory.NewStorage(), memfs.New(), &git.CloneOptions{ + URL: s.url, + Auth: auth, + ReferenceName: s.ref, + SingleBranch: true, + Depth: 1, + Tags: git.NoTags, + RecurseSubmodules: git.NoRecurseSubmodules, + }) + return + }) + if err != nil { + return nil, plumbing.ZeroHash, fmt.Errorf("cannot fetch %s at %s: %w", s.url, s.describe(), err) + } + head, err := repo.Head() + if err != nil { + return nil, plumbing.ZeroHash, err + } + wt, err := repo.Worktree() + if err != nil { + return nil, plumbing.ZeroHash, err + } + return wt.Filesystem, head.Hash(), nil +} + +func fetchGitCommit(ctx context.Context, url string, hash plumbing.Hash, auth transport.AuthMethod) (*git.Repository, error) { + repo, err := git.Init(memory.NewStorage(), memfs.New()) + if err != nil { + return nil, err + } + remote, err := repo.CreateRemote(&config.RemoteConfig{ + Name: git.DefaultRemoteName, + URLs: []string{url}, + }) + if err != nil { + return nil, err + } + // Fetching a hash directly requires the server to allow it + // (uploadpack.allowReachableSHA1InWant), as the common hosts do. + err = remote.FetchContext(ctx, &git.FetchOptions{ + Auth: auth, + Depth: 1, + Tags: git.NoTags, + RefSpecs: []config.RefSpec{ + config.RefSpec(hash.String() + ":" + plumbing.NewRemoteReferenceName(git.DefaultRemoteName, "source").String()), + }, + }) + if err != nil { + return nil, err + } + wt, err := repo.Worktree() + if err != nil { + return nil, err + } + if err = wt.Checkout(&git.CheckoutOptions{Hash: hash}); err != nil { + return nil, err + } + return repo, nil +} diff --git a/pkg/functions/function_git_test.go b/pkg/functions/function_git_test.go new file mode 100644 index 0000000000..1ebee3d283 --- /dev/null +++ b/pkg/functions/function_git_test.go @@ -0,0 +1,188 @@ +package functions_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + fn "knative.dev/func/pkg/functions" +) + +// gitFixture is a local repository served over file:// with: +// - main: func.yaml naming "root-fn" and sub/func.yaml naming "sub-fn" +// - tag v1 (annotated) on the first commit of main +// - branch feature: func.yaml naming "feature-fn" +// - branch v1, at feature's head: the same name as the tag, another commit +// - branch legacy: a func.yaml from spec version 0.25.0, needing migration +type gitFixture struct { + url string + main string // full hash of main's head + feature string // full hash of feature's head +} + +func newGitFixture(t *testing.T) gitFixture { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("No 'git' found in path. Skipping test.") + } + dir := t.TempDir() + run := func(args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) + } + write := func(rel, name string) { + t.Helper() + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + content := "specVersion: " + fn.LastSpecVersion() + "\nname: " + name + "\nruntime: go\ncreated: 2024-01-01T00:00:00Z\n" + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + + run("init", "-q", "-b", "main") + // Let the test fetch a bare commit hash, as the common git hosts allow. + run("config", "uploadpack.allowAnySHA1InWant", "true") + write("func.yaml", "root-fn") + write("sub/func.yaml", "sub-fn") + run("add", ".") + run("commit", "-q", "-m", "initial") + run("tag", "-a", "v1", "-m", "v1") + main := run("rev-parse", "HEAD") + + run("checkout", "-q", "-b", "feature") + write("func.yaml", "feature-fn") + run("commit", "-q", "-am", "feature") + feature := run("rev-parse", "HEAD") + run("branch", "v1") // same name as the tag, a different commit + + run("checkout", "-q", "-b", "legacy", "main") + legacy, err := os.ReadFile(filepath.Join("testdata", "migrations", "v0.34.0", "func.yaml")) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "func.yaml"), legacy, 0o644); err != nil { + t.Fatal(err) + } + run("commit", "-q", "-am", "legacy") + run("checkout", "-q", "main") + + return gitFixture{url: "file://" + dir, main: main, feature: feature} +} + +// TestNewFunctionFromGit ensures a function is loaded from the func.yaml of +// the requested revision and context directory without a local checkout. +func TestNewFunctionFromGit(t *testing.T) { + fx := newGitFixture(t) + tests := []struct { + name string + git fn.Source + wantName string + wantCommit string + }{ + {"default branch", fn.Source{URL: fx.url}, "root-fn", fx.main}, + {"context dir", fn.Source{URL: fx.url, Dir: "sub"}, "sub-fn", fx.main}, + {"branch", fn.Source{URL: fx.url, Revision: "feature"}, "feature-fn", fx.feature}, + {"annotated tag", fn.Source{URL: fx.url, Revision: "v1"}, "root-fn", fx.main}, + {"full ref", fn.Source{URL: fx.url, Revision: "refs/heads/feature"}, "feature-fn", fx.feature}, + {"commit hash", fn.Source{URL: fx.url, Revision: fx.feature}, "feature-fn", fx.feature}, + // A tag wins over a branch of the same name, as for git; the full ref + // name chooses the branch + {"tag over branch", fn.Source{URL: fx.url, Revision: "v1"}, "root-fn", fx.main}, + {"branch by full ref", fn.Source{URL: fx.url, Revision: "refs/heads/v1"}, "feature-fn", fx.feature}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, err := fn.NewFunctionFromGit(context.Background(), tt.git) + if err != nil { + t.Fatal(err) + } + if f.Name != tt.wantName { + t.Errorf("expected name %q, got %q", tt.wantName, f.Name) + } + if f.Build.Source.Commit != tt.wantCommit { + t.Errorf("expected the function read at %s, got %q", tt.wantCommit, f.Build.Source.Commit) + } + // The function records where it was read from + want := tt.git + want.Commit = tt.wantCommit + if f.Build.Source != want { + t.Errorf("expected source %+v, got %+v", want, f.Build.Source) + } + if f.Runtime != "go" { + t.Errorf("expected runtime go, got %q", f.Runtime) + } + if f.Root != "" { + t.Errorf("expected no root, got %q", f.Root) + } + if !f.Initialized() { + t.Error("expected the function to be initialized") + } + }) + } +} + +// TestNewFunctionFromGit_Migrates ensures a func.yaml of an earlier spec +// version is migrated on load, as NewFunction does for a local checkout: +// migrations read the previous structure from the fetched bytes. +func TestNewFunctionFromGit_Migrates(t *testing.T) { + fx := newGitFixture(t) + f, err := fn.NewFunctionFromGit(context.Background(), fn.Source{URL: fx.url, Revision: "legacy"}) + if err != nil { + t.Fatal(err) + } + if f.SpecVersion != fn.LastSpecVersion() { + t.Errorf("expected spec version %q, got %q", fn.LastSpecVersion(), f.SpecVersion) + } + if f.Name != "testfunc" { + t.Errorf("expected name testfunc, got %q", f.Name) + } + // The legacy func.yaml names another repository (http://test-url, moved + // into the source by migrateToSpecsStructure). The loaded function + // records where it was actually read from instead. + if f.Build.Source.URL != fx.url || f.Build.Source.Revision != "legacy" { + t.Errorf("expected the source it was read from (%s at legacy), got %+v", fx.url, f.Build.Source) + } +} + +// TestNewFunctionFromGit_Errors ensures unknown revisions and directories +// without a func.yaml are reported, not silently returned as empty functions. +func TestNewFunctionFromGit_Errors(t *testing.T) { + fx := newGitFixture(t) + tests := []struct { + name string + git fn.Source + wantErr string + }{ + {"unknown revision", fn.Source{URL: fx.url, Revision: "nope"}, "not found"}, + {"unknown full ref", fn.Source{URL: fx.url, Revision: "refs/heads/nope"}, "couldn't find remote ref"}, + {"abbreviated hash", fn.Source{URL: fx.url, Revision: fx.feature[:7]}, "full hash"}, + {"missing func.yaml", fn.Source{URL: fx.url, Dir: "nope"}, "no func.yaml"}, + {"no url", fn.Source{}, "required"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := fn.NewFunctionFromGit(context.Background(), tt.git) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("expected error containing %q, got %q", tt.wantErr, err) + } + }) + } +} diff --git a/pkg/functions/function_migrations.go b/pkg/functions/function_migrations.go index 1da8a82b25..abdbe2967f 100644 --- a/pkg/functions/function_migrations.go +++ b/pkg/functions/function_migrations.go @@ -1,7 +1,6 @@ package functions import ( - "errors" "fmt" "os" "path/filepath" @@ -18,20 +17,34 @@ var unknownFieldsOnce sync.Once // version of the function. It is the caller's responsibility to // .Write() the function to persist to disk. Additionally it will warn on // up-to-date spec but wrong func.yaml (eg. extraneous fields) +// +// Migrations need the function as it was serialized, which Migrate reads +// from the func.yaml at f.Root. See migrate for functions without a Root. func (f Function) Migrate() (migrated Function, err error) { + var raw []byte + if f.Root != "" { + if raw, err = os.ReadFile(filepath.Join(f.Root, FunctionFile)); err != nil && !os.IsNotExist(err) { + return f, err + } + } + return f.migrate(raw) +} + +// migrate applies any necessary migrations to f, whose serialized form (the +// content of its func.yaml) is raw. Migrations read the previous structure +// from raw, so a function needs no working tree to be migrated. +func (f Function) migrate(raw []byte) (migrated Function, err error) { // Return immediately if the function indicates it has already been // migrated. if f.Migrated() { - // Already at the latest spec — check for unknown fields - if f.Root != "" { - if bb, readErr := os.ReadFile(filepath.Join(f.Root, FunctionFile)); readErr == nil { - unknownFieldsOnce.Do(func() { - var strict Function - if strictErr := yaml.UnmarshalStrict(bb, &strict); strictErr != nil { - fmt.Fprintf(os.Stderr, "Warning (unknown fields will be ignored):\n %v\n.\n", formatUnmarshalError(strictErr)) - } - }) - } + // Already at the latest spec: check for unknown fields + if raw != nil { + unknownFieldsOnce.Do(func() { + var strict Function + if strictErr := yaml.UnmarshalStrict(raw, &strict); strictErr != nil { + fmt.Fprintf(os.Stderr, "Warning (unknown fields will be ignored):\n %v\n.\n", formatUnmarshalError(strictErr)) + } + }) } return f, nil } @@ -45,7 +58,7 @@ func (f Function) Migrate() (migrated Function, err error) { } // Apply this migration when the function's specVersion is less than that which // the migration will impart. - migrated, err = m.migrate(migrated, m) + migrated, err = m.migrate(migrated, raw, m) if err != nil { return // fail fast on any migration errors } @@ -61,7 +74,20 @@ type migration struct { } // migrator is a function which returns a migrated copy of an inbound function. -type migrator func(Function, migration) (Function, error) +// It receives the function's serialized form to read the previous structure. +type migrator func(f Function, raw []byte, m migration) (Function, error) + +// unmarshalPrevious loads the pertinent parts of a previous schema version +// from the serialized function raw, on behalf of the named migration. +func unmarshalPrevious(raw []byte, migration string, previous interface{}) error { + if raw == nil { + return fmt.Errorf("migration '%s' error: the serialized function is required", migration) + } + if err := yaml.Unmarshal(raw, previous); err != nil { + return fmt.Errorf("migration '%s' error: %w", migration, err) + } + return nil +} // Migrated returns whether the function has been migrated to the highest // level the currently executing system is aware of (or beyond). @@ -126,7 +152,7 @@ var migrations = []migration{ // created stamp. Otherwise, this is an in-memory (new) function that is // currently in the process of being created and as such need not be mutated // to consider this migration having been evaluated. -func migrateToCreationStamp(f Function, m migration) (Function, error) { +func migrateToCreationStamp(f Function, _ []byte, m migration) (Function, error) { // For functions with no creation timestamp, but appear to have been pre- // existing, populate their created stamp and version. // Yes, it's a little gnarly, but bootstrapping into the loveliness of a @@ -173,16 +199,11 @@ func migrateToCreationStamp(f Function, m migration) (Function, error) { // a customized builder image, that value is preserved as the builder image // for the 'pack' builder in the new version (s2i did not exist prior). // See associated unit tests. -func migrateToBuilderImages(f1 Function, m migration) (Function, error) { +func migrateToBuilderImages(f1 Function, raw []byte, m migration) (Function, error) { // Load the function using pertinent parts of the previous version's schema: - f0Filename := filepath.Join(f1.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f1, errors.New("migration 'migrateToBuilderImages' error: " + err.Error()) - } f0 := migrateToBuilderImages_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f1, errors.New("migration 'migrateToBuilderImages' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateToBuilderImages", &f0); err != nil { + return f1, err } // At time of this migration, the default pack builder image for all language @@ -206,18 +227,11 @@ func migrateToBuilderImages(f1 Function, m migration) (Function, error) { // migrateToSpecVersion updates a func.yaml file to use SpecVersion // instead of Version to track the migration numbers -func migrateToSpecVersion(f Function, m migration) (Function, error) { +func migrateToSpecVersion(f Function, raw []byte, m migration) (Function, error) { // Load the function func.yaml file - f0Filename := filepath.Join(f.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f, errors.New("migration 'migrateToSpecVersion' error: " + err.Error()) - } - - // Only handle the Version field if it exists f0 := migrateToSpecVersion_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f, errors.New("migration 'migrateToSpecVersion' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateToSpecVersion", &f0); err != nil { + return f, err } f.SpecVersion = m.version @@ -227,16 +241,11 @@ func migrateToSpecVersion(f Function, m migration) (Function, error) { // migrateToSpecsStructure migration makes sure use the sub-specs structs for build, run and deploy phases. // To avoid unmarshalling issues with the old format this migration needs to be executed first. // Further migrations will operate on this new struct with sub-specs -func migrateToSpecsStructure(f1 Function, m migration) (Function, error) { +func migrateToSpecsStructure(f1 Function, raw []byte, m migration) (Function, error) { // Load the Function using pertinent parts of the previous version's schema: - f0Filename := filepath.Join(f1.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f1, errors.New("migration 'migrateToSpecsStructure' error: " + err.Error()) - } f0 := migrateToSpecs_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f1, errors.New("migration 'migrateToSpecsStructure' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateToSpecsStructure", &f0); err != nil { + return f1, err } if f0.Git.URL != "" { @@ -306,16 +315,11 @@ func migrateToSpecsStructure(f1 Function, m migration) (Function, error) { // file. When Invoke now holds default value (http) it will not show up in // func.yaml as the default value is implicitly expected. Otherwise if Invoke // is non-default value, it will be written in func.yaml. -func migrateFromInvokeStructure(f1 Function, m migration) (Function, error) { +func migrateFromInvokeStructure(f1 Function, raw []byte, m migration) (Function, error) { // Load the Function using pertinent parts of the previous version's schema: - f0Filename := filepath.Join(f1.Root, FunctionFile) - bb, err := os.ReadFile(f0Filename) - if err != nil { - return f1, errors.New("migration 'migrateFromInvokeStructure' error: " + err.Error()) - } f0 := migrateFromInvokeStructure_previousFunction{} - if err = yaml.Unmarshal(bb, &f0); err != nil { - return f1, errors.New("migration 'migrateFromInvokeStructure' error: " + err.Error()) + if err := unmarshalPrevious(raw, "migrateFromInvokeStructure", &f0); err != nil { + return f1, err } if f0.Invocation.Format != "" && f0.Invocation.Format != "http" { @@ -327,13 +331,7 @@ func migrateFromInvokeStructure(f1 Function, m migration) (Function, error) { return f1, nil } -func migratePersistentVolumeTypoFixup(fn Function, m migration) (Function, error) { - f, err := os.Open(filepath.Join(fn.Root, FunctionFile)) - if err != nil { - return Function{}, fmt.Errorf("cannot open func.yaml: %w", err) - } - defer f.Close() - +func migratePersistentVolumeTypoFixup(fn Function, raw []byte, m migration) (Function, error) { data := struct { Run struct { Volumes []struct { @@ -341,11 +339,8 @@ func migratePersistentVolumeTypoFixup(fn Function, m migration) (Function, error } `yaml:"volumes,omitempty"` } }{} - - dec := yaml.NewDecoder(f) - err = dec.Decode(&data) - if err != nil { - return Function{}, fmt.Errorf("cannot deserialize old sub-structure: %w", err) + if err := unmarshalPrevious(raw, "migratePersistentVolumeTypoFixup", &data); err != nil { + return fn, err } for idx, volume := range data.Run.Volumes { @@ -363,20 +358,14 @@ func migratePersistentVolumeTypoFixup(fn Function, m migration) (Function, error // are --source, --revision and --source-dir, and nothing about the values // is specific to git. The old keys are carried over; the next write stores // the new ones. -func migrateGitToSource(fn Function, m migration) (Function, error) { - f, err := os.Open(filepath.Join(fn.Root, FunctionFile)) - if err != nil { - return Function{}, fmt.Errorf("cannot open func.yaml: %w", err) - } - defer f.Close() - +func migrateGitToSource(fn Function, raw []byte, m migration) (Function, error) { // Before the specs structure (0.34.0), build was the build type as a // string, so the key is read loosely. data := struct { Build interface{} `yaml:"build"` }{} - if err = yaml.NewDecoder(f).Decode(&data); err != nil { - return Function{}, fmt.Errorf("cannot deserialize old sub-structure: %w", err) + if err := unmarshalPrevious(raw, "migrateGitToSource", &data); err != nil { + return fn, err } if fn.Build.Source.URL == "" { diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index 564e4ea7f6..dbf5bd9921 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -123,6 +123,12 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return "", f, err } + // The source is either a git repository or the local working tree, which + // is uploaded. A function loaded from git has no Root. + if f.Build.Source.URL == "" && f.Root == "" { + return "", f, errors.New("a local function directory is required to upload sources; set a git URL to build from a repository") + } + // Warn if the func-generated legacy .s2i/bin/assemble exists; it will be // uploaded to the PVC and can interfere with the in-cluster build. // Remote deploy doesn't go through Client.Build, so we re-check here. diff --git a/pkg/pipelines/tekton/templates.go b/pkg/pipelines/tekton/templates.go index 0650ef0dce..ec9717e716 100644 --- a/pkg/pipelines/tekton/templates.go +++ b/pkg/pipelines/tekton/templates.go @@ -451,12 +451,13 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m var manifestivalClient = k8s.GetManifestivalClient // createAndApplyResource tries to create and apply a resource to the k8s cluster from the input template and data, -// if there's the same resource already created in the project directory, it is used instead +// if there's the same resource already created in the project directory, it is used instead. +// An empty projectRoot (a function loaded from git) has no such directory. func createAndApplyResource(projectRoot, fileName, fileTemplate, kind, resourceName, namespace string, data interface{}) error { var source manifestival.Source filePath := path.Join(projectRoot, resourcesDirectory, fileName) - if _, err := os.Stat(filePath); !os.IsNotExist(err) { + if _, err := os.Stat(filePath); projectRoot != "" && !os.IsNotExist(err) { source = manifestival.Path(filePath) } else { tmpl, err := template.New("template").Parse(fileTemplate) diff --git a/pkg/pipelines/tekton/templates_test.go b/pkg/pipelines/tekton/templates_test.go index b09e0fa52d..0831d8c1fd 100644 --- a/pkg/pipelines/tekton/templates_test.go +++ b/pkg/pipelines/tekton/templates_test.go @@ -318,6 +318,35 @@ var testData = []struct { }, } +// Test_createAndApplyPipelineRunTemplate_NoRoot ensures a function loaded +// from a git repository, which has no Root, yields a Pipeline and a +// PipelineRun: nothing is read from a project directory in that case. +func Test_createAndApplyPipelineRunTemplate_NoRoot(t *testing.T) { + old := manifestivalClient + defer func() { manifestivalClient = old }() + manifestivalClient = func() (manifestival.Client, error) { + return fake.New(), nil + } + + f := fn.Function{ + Name: "remote-fn", + Runtime: "go", + Registry: TestRegistry, + Build: fn.BuildSpec{ + Builder: builders.Pack, + Source: fn.Source{URL: "https://example.com/alice/remote-fn.git", Revision: "main"}, + }, + } + f.Deploy.Image = "docker.io/alice/remote-fn" + + if err := createAndApplyPipelineTemplate(f, "test-ns", nil); err != nil { + t.Fatal(err) + } + if err := createAndApplyPipelineRunTemplate(f, "test-ns", nil); err != nil { + t.Fatal(err) + } +} + func Test_createAndApplyPipelineRunTemplate(t *testing.T) { for _, tt := range testData { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/testing/testing.go b/pkg/testing/testing.go index e006b0265b..bf6f1ff054 100644 --- a/pkg/testing/testing.go +++ b/pkg/testing/testing.go @@ -26,6 +26,7 @@ import ( "os/exec" "path/filepath" "runtime" + "sort" "strings" "testing" ) @@ -226,6 +227,65 @@ func WithExecutable(t *testing.T, name, goSrc string) { } } +// ServeGitRepository serves over HTTP, as ServeRepo does, a new repository +// with one commit per branch. branches maps a branch name to the files that +// commit writes (slash-separated path to content); main is created first, +// with its files or none, and every other branch starts from main. Returned +// are the repository's URL and the head of each branch as a full hash. +// Requires the git binary. +func ServeGitRepository(t *testing.T, branches map[string]map[string]string) (url string, heads map[string]string) { + t.Helper() + root := t.TempDir() + work := filepath.Join(t.TempDir(), "work") + if err := os.MkdirAll(work, 0o755); err != nil { + t.Fatal(err) + } + git := func(dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) + } + commit := func(files map[string]string, msg string) string { + t.Helper() + for name, content := range files { + p := filepath.Join(work, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + git(work, "add", "-A") + git(work, "-c", "commit.gpgsign=false", "commit", "-q", "--allow-empty", "-m", msg) + return git(work, "rev-parse", "HEAD") + } + + git(work, "init", "-q", "-b", "main") + heads = map[string]string{"main": commit(branches["main"], "main")} + names := make([]string, 0, len(branches)) + for name := range branches { + if name != "main" { + names = append(names, name) + } + } + sort.Strings(names) + for _, name := range names { + git(work, "checkout", "-q", "-b", name, "main") + heads[name] = commit(branches[name], name) + } + git(root, "clone", "-q", "--bare", work, "repository.git") + return RunGitServer(root, t) + "/repository.git", heads +} + // RunGitServer starts serving git HTTP server and returns its address func RunGitServer(root string, t *testing.T) (url string) { l, err := net.Listen("tcp", "127.0.0.1:0") From d0f44cfa4d6a84fae6be65c4cbfe08bd1cb1def1 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 3 Sep 2026 00:22:55 +0200 Subject: [PATCH 4/7] fix: allow rebuilding a function from git on the same volume The git-clone StepAction runs as user 65532 and empties the source workspace before cloning. The previous run of the pipeline leaves the sources there owned by the build user (the prepare step chowns the tree to 1001 for the buildpacks lifecycle), which 65532 can neither delete nor create .git next to. Every remote build of a function from git after its first therefore failed in the fetch step, until func delete removed the volume. A clean-src step, run as root and gated on a git URL like fetch-src, now empties the workspace and hands the directory to the clone user before the clone. The upload path is unaffected, and the cache workspace is a separate directory that is left alone. --- pkg/functions/function_git_test.go | 6 ++++++ pkg/pipelines/tekton/task-buildpack.yaml.tmpl | 17 +++++++++++++++++ pkg/pipelines/tekton/task-s2i.yaml.tmpl | 17 +++++++++++++++++ pkg/pipelines/tekton/tasks_test.go | 19 +++++++++++++++++++ 4 files changed, 59 insertions(+) diff --git a/pkg/functions/function_git_test.go b/pkg/functions/function_git_test.go index 1ebee3d283..6555e3144e 100644 --- a/pkg/functions/function_git_test.go +++ b/pkg/functions/function_git_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" @@ -25,6 +26,11 @@ type gitFixture struct { func newGitFixture(t *testing.T) gitFixture { t.Helper() + if runtime.GOOS == "windows" { + // The fixture is served over file://, which go-git only supports + // through the git binary and not from a Windows path. + t.Skip("file:// repositories are not supported on Windows") + } if _, err := exec.LookPath("git"); err != nil { t.Skip("No 'git' found in path. Skipping test.") } diff --git a/pkg/pipelines/tekton/task-buildpack.yaml.tmpl b/pkg/pipelines/tekton/task-buildpack.yaml.tmpl index c60e2c06df..be025e73db 100644 --- a/pkg/pipelines/tekton/task-buildpack.yaml.tmpl +++ b/pkg/pipelines/tekton/task-buildpack.yaml.tmpl @@ -72,6 +72,23 @@ spec: - name: CNB_PLATFORM_API value: "0.10" steps: + - name: clean-src + # The git-clone StepAction runs as user 65532 and deletes what is in the + # workspace before cloning. A previous run of this pipeline leaves the + # sources there owned by the build user, which 65532 cannot remove, so a + # second build of the same function from git failed. Empty the workspace + # as root first and hand the directory to the clone user. + image: docker.io/library/bash:5.1 + when: + - input: "$(params.GIT_REPOSITORY)" + operator: notin + values: [""] + script: | + #!/usr/bin/env bash + set -e + src="$(workspaces.source.path)" + find "$src" -mindepth 1 -delete + chown 65532:65532 "$src" - name: fetch-src ref: resolver: bundles diff --git a/pkg/pipelines/tekton/task-s2i.yaml.tmpl b/pkg/pipelines/tekton/task-s2i.yaml.tmpl index c739a45a1f..5ea3335222 100644 --- a/pkg/pipelines/tekton/task-s2i.yaml.tmpl +++ b/pkg/pipelines/tekton/task-s2i.yaml.tmpl @@ -57,6 +57,23 @@ spec: An optional workspace that allows providing a .docker/config.json file for Buildah to access the container registry. The file should be placed at the root of the Workspace with name config.json. optional: true steps: + - name: clean-src + # The git-clone StepAction runs as user 65532 and deletes what is in the + # workspace before cloning. A previous run of this pipeline leaves the + # sources there owned by the build user, which 65532 cannot remove, so a + # second build of the same function from git failed. Empty the workspace + # as root first and hand the directory to the clone user. + image: docker.io/library/bash:5.1 + when: + - input: "$(params.GIT_REPOSITORY)" + operator: notin + values: [""] + script: | + #!/usr/bin/env bash + set -e + src="$(workspaces.source.path)" + find "$src" -mindepth 1 -delete + chown 65532:65532 "$src" - name: fetch-src ref: resolver: bundles diff --git a/pkg/pipelines/tekton/tasks_test.go b/pkg/pipelines/tekton/tasks_test.go index f4fca3284e..95d7c74fe9 100644 --- a/pkg/pipelines/tekton/tasks_test.go +++ b/pkg/pipelines/tekton/tasks_test.go @@ -61,6 +61,25 @@ func TestGetTasks(t *testing.T) { if apiErr != nil { t.Fatalf("%+v\n", apiErr) } + + // The workspace is emptied as root right before the clone, and + // only when there is a repository to clone; the upload path must + // keep the sources it received. + steps := task.Spec.Steps + if len(steps) < 2 || steps[0].Name != "clean-src" || steps[1].Name != "fetch-src" { + t.Fatalf("expected clean-src to precede fetch-src, got %v", stepNames(steps)) + } + if len(steps[0].When) != 1 || steps[0].When[0].Input != "$(params.GIT_REPOSITORY)" { + t.Errorf("expected clean-src to be gated on GIT_REPOSITORY, got %+v", steps[0].When) + } }) } } + +func stepNames(steps []tektonv1.Step) []string { + names := make([]string, 0, len(steps)) + for _, s := range steps { + names = append(names, s.Name) + } + return names +} From 0be587983ad701bea164a6a5bcf3267bbc83402c Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 3 Sep 2026 01:01:39 +0200 Subject: [PATCH 5/7] fix: default deploy flags from the repository's function The flag defaults of deploy are derived from the function in the current directory when the command is built, so a local func.yaml is honoured unless a flag or environment variable overrides it. A function read from a git repository is known only at run time and got no such treatment: from an empty directory the static defaults applied, and Configure then overwrote, for example, the repository's s2i builder with pack. withFunctionDefaults gives the config the defaults NewDeployCmd would have registered had the repository's function been the local one, for every flag the user did not set. The reload after the prompt now also happens when the prompt changed the git source of a function that already came from git, not only when it made a local function remote. --- cmd/deploy.go | 77 ++++++++++++++++++++++++++++++++++++++++++++-- cmd/deploy_test.go | 54 ++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 16783ce130..94c991e40a 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "os" "strconv" "strings" @@ -281,6 +282,10 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { if f, err = cfg.function(cmd.Context(), local); err != nil { return } + if f.Root == "" { + cfg = cfg.withFunctionDefaults(cmd, f) + } + source := cfg.gitSource() // Now that we know function exists, proceed with prompting if cfg, err = cfg.Prompt(f); err != nil { @@ -292,11 +297,13 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { if err = cfg.Validate(cmd); err != nil { return wrapValidateError(err, "deploy") } - // The prompt may have made the source a git repository - if cfg.Remote && cfg.Source != "" && f.Root != "" { + // The prompt may have chosen a git repository as the source, or another + // one than the function was read from. + if cfg.Remote && cfg.Source != "" && (f.Root != "" || cfg.gitSource() != source) { if f, err = cfg.function(cmd.Context(), local); err != nil { return } + cfg = cfg.withFunctionDefaults(cmd, f) } // Warn if registry changed but registryInsecure is still true @@ -484,6 +491,72 @@ func (c deployConfig) function(ctx context.Context, local fn.Function) (fn.Funct return local, nil } +// withFunctionDefaults returns the config with the defaults it would have had +// if f were the function in the current directory when the command was +// built: the values NewDeployCmd registers as flag defaults from the function +// with context, for every flag the user did not set on the command line or in +// the environment. It is used when the function comes from a git repository, +// which is known only at run time, so that its func.yaml is honoured the same +// way a local one is. +func (c deployConfig) withFunctionDefaults(cmd *cobra.Command, f fn.Function) deployConfig { + global, err := config.NewDefault() + if err != nil { + fmt.Fprintf(cmd.OutOrStdout(), "error loading config at '%v'. %v\n", config.File(), err) + } + global = global.Apply(f) + + unset := func(flag string) bool { + _, env := os.LookupEnv("FUNC_" + strings.ToUpper(strings.ReplaceAll(flag, "-", "_"))) + return !cmd.Flags().Changed(flag) && !env + } + if unset("builder") { + c.Builder = global.Builder + } + if unset("deployer") { + c.Deployer = global.Deployer + } + if unset("registry") { + c.Registry = global.Registry + } + if unset("registry-insecure") { + c.RegistryInsecure = global.RegistryInsecure + } + if unset("builder-image") { + c.BuilderImage = f.Build.BuilderImages[c.Builder] + } + if unset("base-image") { + c.BaseImage = f.Build.BaseImage + } + if unset("image") { + c.Image = f.Image + } + if unset("domain") { + c.Domain = f.Domain + } + if unset("remote-storage-class") { + c.RemoteStorageClass = f.Build.RemoteStorageClass + } + if unset("pvc-size") { + c.PVCSize = f.Build.PVCSize + } + if unset("service-account") { + c.ServiceAccountName = f.Deploy.ServiceAccountName + } + if unset("image-pull-secret") { + c.ImagePullSecret = f.Deploy.ImagePullSecret + } + if unset("expose") { + c.Expose = f.Expose + } + if unset("namespace") { + c.Namespace = defaultNamespace(f, c.Verbose) + } + if unset("management-disabled") { + c.ManagementDisabled = f.Deploy.ManagementDisabled + } + return c +} + // gitSource is the git repository to build from, as configured: the URL // may carry the revision as a fragment (#), which then // wins, as it does in Configure. diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 2b2f978529..9e0e50ae72 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -660,6 +660,60 @@ func TestDeploy_RemoteGitUsesRepositoryFunction(t *testing.T) { } } +// TestDeploy_RemoteGitDefaultsFromRepository ensures the flag defaults come +// from the repository's func.yaml, as they come from a local func.yaml, with +// flags and environment variables still taking precedence. The command's +// flag defaults were derived from the (here empty) current directory. +func TestDeploy_RemoteGitDefaultsFromRepository(t *testing.T) { + _ = FromTempDirectory(t) + url, _ := ServeGitRepository(t, map[string]map[string]string{ + "main": {"func.yaml": funcYAML("remote-fn", `registry: example.com/repo +deployer: raw +build: + builder: s2i +deploy: + namespace: repo-ns + serviceAccountName: repo-sa +`)}, + }) + + deploy := func(t *testing.T, args ...string) fn.Function { + t.Helper() + var deployed fn.Function + pipeliner := mock.NewPipelinesProvider() + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + deployed = f + return base(f) + } + cmd := NewDeployCmd(NewTestClient(fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry))) + cmd.SetArgs(append([]string{"--remote", "--source=" + url}, args...)) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + return deployed + } + + // Nothing set: the repository's values apply + f := deploy(t) + if f.Build.Builder != "s2i" || f.Registry != "example.com/repo" || f.Deployer != "raw" || + f.Deploy.ServiceAccountName != "repo-sa" || f.Namespace != "repo-ns" { + t.Errorf("expected the repository's func.yaml to provide the defaults, got builder=%q registry=%q deployer=%q sa=%q namespace=%q", + f.Build.Builder, f.Registry, f.Deployer, f.Deploy.ServiceAccountName, f.Namespace) + } + + // Flags and environment variables win over the repository + t.Setenv("FUNC_DEPLOYER", "knative") + f = deploy(t, "--builder=pack", "--registry=example.com/flag", "--namespace=flag-ns") + if f.Build.Builder != "pack" || f.Registry != "example.com/flag" || f.Deployer != "knative" || f.Namespace != "flag-ns" { + t.Errorf("expected flags and env to win, got builder=%q registry=%q deployer=%q namespace=%q", + f.Build.Builder, f.Registry, f.Deployer, f.Namespace) + } + if f.Deploy.ServiceAccountName != "repo-sa" { + t.Errorf("expected the untouched service account to stay the repository's, got %q", f.Deploy.ServiceAccountName) + } +} + // TestDeploy_RemoteGitLoadError ensures a repository the function cannot be // read from fails the deployment before any pipeline is run. func TestDeploy_RemoteGitLoadError(t *testing.T) { From 5f4f94cbaf1b6256d723fe719dbc4c856ee8644e Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 3 Sep 2026 08:01:38 +0200 Subject: [PATCH 6/7] feat: deploy --remote --source writes nothing locally A remote deployment of a git repository recorded the git settings and the outcome (image, namespace, deployer, exposure) on whatever local function was at the path, a habit from when the local func.yaml was the source of such a deployment. It was wrong for a checkout on another branch, misleading for a different function, and it stamped sources that were never built. Such a deployment is now a deployment by reference: the function is read from the repository, deployed, and nothing is written, neither to the repository nor to the current directory. To change a function that lives in a repository, clone it, edit it and deploy the working tree. A func.yaml may still carry build.source settings, which serve as the defaults of the --source flags. Loading is one function, loadFunction, without a local function to consult. --- cmd/deploy.go | 87 +++++++--------- cmd/deploy_test.go | 181 +++++++++++++--------------------- docs/reference/func_deploy.md | 4 +- 3 files changed, 105 insertions(+), 167 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 94c991e40a..ae3218d409 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -1,7 +1,6 @@ package cmd import ( - "context" "errors" "fmt" "io" @@ -85,7 +84,9 @@ DESCRIPTION A branch, tag or commit is given with '--revision': '{{rootCmdUse}} deploy --remote --source=git.example.com/alice/f.git --revision=v1.2.0' The function is then read from the repository, so no local copy is - needed. Choose the directory within the repository with '--source-dir'. + needed, and nothing is written locally: to change the function, clone + the repository, edit it and deploy the working tree. Choose the + directory within the repository with '--source-dir'. Domain When deploying, a function's route is automatically generated using the @@ -266,25 +267,17 @@ EXAMPLES func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { var ( - cfg deployConfig - f fn.Function // the function to deploy - local fn.Function // the function at cfg.Path, if any + cfg deployConfig + f fn.Function ) // Initialize config first cfg = newDeployConfig(cmd) - // Load the function at path. It is the function to deploy unless the - // source is a git repository, in which case it only records the outcome. - if local, err = fn.NewFunction(cfg.Path); err != nil { + // Load the function to deploy + if f, cfg, err = cfg.loadFunction(cmd); err != nil { return } - if f, err = cfg.function(cmd.Context(), local); err != nil { - return - } - if f.Root == "" { - cfg = cfg.withFunctionDefaults(cmd, f) - } source := cfg.gitSource() // Now that we know function exists, proceed with prompting @@ -300,10 +293,9 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // The prompt may have chosen a git repository as the source, or another // one than the function was read from. if cfg.Remote && cfg.Source != "" && (f.Root != "" || cfg.gitSource() != source) { - if f, err = cfg.function(cmd.Context(), local); err != nil { + if f, cfg, err = cfg.loadFunction(cmd); err != nil { return } - cfg = cfg.withFunctionDefaults(cmd, f) } // Warn if registry changed but registryInsecure is still true @@ -447,23 +439,12 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { } // Write - // A function deployed from a git repository has no working tree of its own. - // A local function at path, if there is one, records the request and the - // outcome so that later commands (describe, delete, another deploy) find - // them; its own metadata is left alone. + // A function deployed from a git repository is deployed by reference: + // nothing is written, neither to the repository nor to whatever is in the + // current directory. To change such a function, clone the repository, + // edit it and deploy the working tree. if f.Root == "" { - if !local.Initialized() { - return nil - } - if local, err = cfg.Configure(local); err != nil { - return - } - local.Registry = f.Registry - local.Deploy.Image = f.Deploy.Image - local.Deploy.Namespace = f.Deploy.Namespace - local.Deploy.Deployer = f.Deploy.Deployer - local.Deploy.Expose = f.Deploy.Expose - f = local + return nil } if err = f.Write(); err != nil { return @@ -476,19 +457,28 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return f.Stamp() } -// function returns the function to deploy. When a git repository is the +// loadFunction returns the function to deploy, and the config completed +// with the defaults that function provides. When a git repository is the // source of a remote deployment it is the function committed there: the -// pipeline must describe what the cluster builds, and a local checkout may -// be absent, on another branch or in another directory. Otherwise it is the -// given local function, which must be initialized. -func (c deployConfig) function(ctx context.Context, local fn.Function) (fn.Function, error) { +// pipeline must describe what the cluster builds, and the current directory +// plays no part. Otherwise it is the function at the path, which must be +// initialized. +func (c deployConfig) loadFunction(cmd *cobra.Command) (fn.Function, deployConfig, error) { if c.Remote && c.Source != "" { - return fn.NewFunctionFromGit(ctx, c.gitSource()) + f, err := fn.NewFunctionFromGit(cmd.Context(), c.gitSource()) + if err != nil { + return f, c, err + } + return f, c.withFunctionDefaults(cmd, f), nil } - if !local.Initialized() { - return local, NewErrNotInitializedFromPath(local.Root, "deploy") + f, err := fn.NewFunction(c.Path) + if err != nil { + return f, c, err + } + if !f.Initialized() { + return f, c, NewErrNotInitializedFromPath(f.Root, "deploy") } - return local, nil + return f, c, nil } // withFunctionDefaults returns the config with the defaults it would have had @@ -1023,17 +1013,10 @@ func printDeployMessages(out io.Writer, f fn.Function) { // current invocation is not remote. (providing Git attributes directly // via flags without --remote will error elsewhere). // - // When invoking a remote build with --remote, the --git-X arguments - // are persisted to the local function's source code such that the reference - // is retained. Subsequent runs of deploy then need not have these arguments - // present. - // - // However, when building _locally_ thereafter, the deploy command should - // prefer the local source code, ignoring the values for --source etc. - // Since this might be confusing, a warning is issued below that the local - // function source does include a reference to a git repository, but that it - // will be ignored in favor of the local source code since --remote was not - // specified. + // A func.yaml may carry source settings (build.source), which then serve + // as the defaults of the --source flags. When building _locally_, the deploy + // command uses the local source code and ignores them. Since this might be + // confusing, a warning is issued below. if !f.Local.Remote && (f.Build.Source.URL != "" || f.Build.Source.Revision != "" || f.Build.Source.Dir != "") { fmt.Fprintf(out, "Warning: source settings are only applicable when running with --remote. Local source code will be used.") } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 9e0e50ae72..a8af317903 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -609,25 +609,36 @@ func TestDeploy_RemoteGitNoLocalFunction(t *testing.T) { } } -// TestDeploy_RemoteGitUsesRepositoryFunction ensures that, when a local -// function exists alongside a remote deployment of a git repository, the -// pipeline is created for the function as committed in the repository, -// while the local function records the request and the outcome. -func TestDeploy_RemoteGitUsesRepositoryFunction(t *testing.T) { +// TestDeploy_RemoteGitWritesNothing ensures a remote deployment of a git +// repository is a deployment by reference: the pipeline is created for the +// function as committed in the repository, and whatever function is in the +// current directory, even a copy of the same one, is neither written to nor +// marked as built. +func TestDeploy_RemoteGitWritesNothing(t *testing.T) { root := FromTempDirectory(t) - url := serveFunction(t, "remote-fn") + // Only the repository's copy of my-fn has this environment variable + url, _ := ServeGitRepository(t, map[string]map[string]string{ + "main": {"func.yaml": funcYAML("my-fn", `run: + envs: + - name: REPO_ONLY + value: "yes" +`)}, + }) - if _, err := fn.New().Init(fn.Function{Name: "local-fn", Runtime: "node", Root: root}); err != nil { + if _, err := fn.New().Init(fn.Function{Name: "my-fn", Runtime: "go", Root: root}); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(filepath.Join(root, fn.FunctionFile)) + if err != nil { t.Fatal(err) } pipeliner := mock.NewPipelinesProvider() - var deployed fn.Function // as returned by the pipeline: the outcome + var deployed fn.Function base := pipeliner.RunFn pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { - url, f, err := base(f) deployed = f - return url, f, err + return base(f) } cmd := NewDeployCmd(NewTestClient( fn.WithPipelinesProvider(pipeliner), @@ -638,25 +649,22 @@ func TestDeploy_RemoteGitUsesRepositoryFunction(t *testing.T) { t.Fatal(err) } - if deployed.Name != "remote-fn" || deployed.Runtime != "go" { - t.Errorf("expected the repository's function to be deployed, got %q (%v)", deployed.Name, deployed.Runtime) + if len(deployed.Run.Envs) != 1 { + t.Errorf("expected the repository's function to be deployed, got envs %v", deployed.Run.Envs) } - - local, err := fn.NewFunction(root) + after, err := os.ReadFile(filepath.Join(root, fn.FunctionFile)) if err != nil { t.Fatal(err) } - if local.Name != "local-fn" || local.Runtime != "node" { - t.Errorf("expected the local function's metadata to be untouched, got %q (%v)", local.Name, local.Runtime) + if string(before) != string(after) { + t.Errorf("expected the local function to be untouched, got:\n%s", after) } - if local.Build.Source.URL != url { - t.Errorf("expected the git source to be recorded locally, got %q", local.Build.Source.URL) - } - if local.Deploy.Namespace != "fnns" { - t.Errorf("expected the deployed namespace to be recorded locally, got %q", local.Deploy.Namespace) + local, err := fn.NewFunction(root) + if err != nil { + t.Fatal(err) } - if local.Deploy.Image != deployed.Deploy.Image || local.Deploy.Image == "" { - t.Errorf("expected the deployed image %q to be recorded locally, got %q", deployed.Deploy.Image, local.Deploy.Image) + if local.Built() { + t.Error("expected the local sources, which were not built, not to be stamped as built") } } @@ -732,63 +740,29 @@ func TestDeploy_RemoteGitLoadError(t *testing.T) { } } -// TestDeploy_RemoteSourcePersists ensures that the source flags, if provided, -// are persisted to the Function for subsequent deployments. -func TestDeploy_RemoteSourcePersists(t *testing.T) { - root := FromTempDirectory(t) - - var ( - branch = "main" - dir = "function" - ) - url, _ := ServeGitRepository(t, map[string]map[string]string{ - "main": {"function/func.yaml": funcYAML("repo-fn", "")}, - }) - - // Create a new Function in the temp directory - f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) - if err != nil { - t.Fatal(err) - } - - // Deploy the Function specifying all of the source flags - cmd := NewDeployCmd(NewTestClient( - fn.WithPipelinesProvider(mock.NewPipelinesProvider()), - fn.WithRegistry(TestRegistry), - )) - cmd.SetArgs([]string{"--remote", "--source=" + url, "--revision=" + branch, "--source-dir=" + dir, "."}) - if err := cmd.Execute(); err != nil { - t.Fatal(err) - } - - // Load the Function and ensure the flags were stored. - f, err = fn.NewFunction(root) - if err != nil { - t.Fatal(err) - } - if want := (fn.Source{URL: url, Revision: branch, Dir: dir}); f.Build.Source != want { - t.Errorf("expected source %+v persisted, got %+v", want, f.Build.Source) - } -} - // TestDeploy_RemoteSourceEnv ensures the source flags are also read from // their environment variables, FUNC_SOURCE, FUNC_REVISION and -// FUNC_SOURCE_DIR. +// FUNC_SOURCE_DIR: the function is read from the repository they name. func TestDeploy_RemoteSourceEnv(t *testing.T) { - root := FromTempDirectory(t) - - if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { - t.Fatal(err) - } - url, _ := ServeGitRepository(t, map[string]map[string]string{ + _ = FromTempDirectory(t) + // Only the named branch and directory hold a function named repo-fn + url, heads := ServeGitRepository(t, map[string]map[string]string{ + "main": {"func.yaml": funcYAML("root-fn", "")}, "release": {"function/func.yaml": funcYAML("repo-fn", "")}, }) t.Setenv("FUNC_SOURCE", url) t.Setenv("FUNC_REVISION", "release") t.Setenv("FUNC_SOURCE_DIR", "function") + pipeliner := mock.NewPipelinesProvider() + var deployed fn.Function + base := pipeliner.RunFn + pipeliner.RunFn = func(f fn.Function) (string, fn.Function, error) { + deployed = f + return base(f) + } cmd := NewDeployCmd(NewTestClient( - fn.WithPipelinesProvider(mock.NewPipelinesProvider()), + fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry), )) cmd.SetArgs([]string{"--remote"}) @@ -796,13 +770,9 @@ func TestDeploy_RemoteSourceEnv(t *testing.T) { t.Fatal(err) } - f, err := fn.NewFunction(root) - if err != nil { - t.Fatal(err) - } - want := fn.Source{URL: url, Revision: "release", Dir: "function"} - if f.Build.Source != want { - t.Errorf("expected source %+v from the environment, got %+v", want, f.Build.Source) + want := fn.Source{URL: url, Revision: "release", Dir: "function", Commit: heads["release"]} + if deployed.Name != "repo-fn" || deployed.Build.Source != want { + t.Errorf("expected repo-fn read from %+v (the environment), got %q from %+v", want, deployed.Name, deployed.Build.Source) } } @@ -863,15 +833,11 @@ func TestDeploy_RemoteSourceUsed(t *testing.T) { } } -// TestDeploy_RemoteSourceFragment ensures that a --source which specifies the branch -// in the URL is equivalent to providing --revision +// TestDeploy_RemoteSourceFragment ensures that a --source which specifies the +// branch in the URL is equivalent to providing --revision: the function is +// read from, and the pipeline builds, the repository at that branch. func TestDeploy_RemoteSourceFragment(t *testing.T) { - root := FromTempDirectory(t) - - f, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}) - if err != nil { - t.Fatal(err) - } + _ = FromTempDirectory(t) // Only the branch named in the fragment holds a function named repo-fn expectedUrl, _ := ServeGitRepository(t, map[string]map[string]string{ @@ -890,12 +856,10 @@ func TestDeploy_RemoteSourceFragment(t *testing.T) { return base(f) } cmd := NewDeployCmd(NewTestClient( - fn.WithDeployer(mock.NewDeployer()), - fn.WithBuilder(mock.NewBuilder()), fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry), )) - cmd.SetArgs([]string{"--remote", "--source=" + url}) + cmd.SetArgs([]string{"--remote", "--source=" + url, "--namespace=fnns"}) if err := cmd.Execute(); err != nil { t.Fatal(err) @@ -903,16 +867,8 @@ func TestDeploy_RemoteSourceFragment(t *testing.T) { if deployed.Name != "repo-fn" { t.Errorf("expected the function to be read from %q at %q, got %q", expectedUrl, expectedBranch, deployed.Name) } - - f, err = fn.NewFunction(root) - if err != nil { - t.Fatal(err) - } - if f.Build.Source.URL != expectedUrl { - t.Errorf("expected git URL '%v' got '%v'", expectedUrl, f.Build.Source.URL) - } - if f.Build.Source.Revision != expectedBranch { - t.Errorf("expected git branch '%v' got '%v'", expectedBranch, f.Build.Source.Revision) + if deployed.Build.Source.URL != expectedUrl || deployed.Build.Source.Revision != expectedBranch { + t.Errorf("expected the pipeline to build %q at %q, got %+v", expectedUrl, expectedBranch, deployed.Build.Source) } } @@ -2089,37 +2045,37 @@ func TestDeploy_UnsetFlag(t *testing.T) { t.Fatal(err) } - // Deploy it, specifying a Git URL - url := serveFunction(t, "f") - cmd := NewDeployCmd(NewTestClient()) - cmd.SetArgs([]string{"--remote", "--source=" + url}) + // Deploy it, specifying a domain + client := NewTestClient(fn.WithDeployer(mock.NewDeployer()), fn.WithBuilder(mock.NewBuilder()), fn.WithRegistry(TestRegistry)) + cmd := NewDeployCmd(client) + cmd.SetArgs([]string{"--domain=example.com"}) if err := cmd.Execute(); err != nil { t.Fatal(err) } - // Load the function and confirm the URL was persisted + // Load the function and confirm the domain was persisted f, err = fn.NewFunction(root) if err != nil { t.Fatal(err) } - if f.Build.Source.URL != url { - t.Fatalf("url not persisted") + if f.Domain != "example.com" { + t.Fatalf("domain not persisted") } // Deploy it again, unsetting the value - cmd = NewDeployCmd(NewTestClient()) - cmd.SetArgs([]string{"--source="}) + cmd = NewDeployCmd(client) + cmd.SetArgs([]string{"--domain="}) if err := cmd.Execute(); err != nil { t.Fatal(err) } - // Load the function and confirm the URL was unset + // Load the function and confirm the domain was unset f, err = fn.NewFunction(root) if err != nil { t.Fatal(err) } - if f.Build.Source.URL != "" { - t.Fatalf("url not cleared") + if f.Domain != "" { + t.Fatalf("domain not cleared") } } @@ -3282,7 +3238,6 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { root := FromTempDirectory(t) cleanup := k8s.SetOpenShiftForTest(true, nil) defer cleanup() - url := serveFunction(t, "repo-fn") if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) @@ -3308,9 +3263,7 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { var out bytes.Buffer cmd.SetOut(&out) cmd.SetErr(&out) - cmd.SetArgs([]string{"--remote", - "--source=" + url, - "--deployer=raw", "--expose=route"}) + cmd.SetArgs([]string{"--remote", "--deployer=raw", "--expose=route"}) if err := cmd.Execute(); err != nil { t.Fatal(err) } diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index 4fc166c678..01e43a03dd 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -60,7 +60,9 @@ DESCRIPTION A branch, tag or commit is given with '--revision': 'func deploy --remote --source=git.example.com/alice/f.git --revision=v1.2.0' The function is then read from the repository, so no local copy is - needed. Choose the directory within the repository with '--source-dir'. + needed, and nothing is written locally: to change the function, clone + the repository, edit it and deploy the working tree. Choose the + directory within the repository with '--source-dir'. Domain When deploying, a function's route is automatically generated using the From 535e5cfaa677c27a4b00ccc6e1a3f959aed93773 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 23 Sep 2026 15:23:04 +0200 Subject: [PATCH 7/7] wip: dedicated deploy path for --remote --source --- cmd/build.go | 29 ++-- cmd/config_git_set.go | 2 +- cmd/deploy.go | 284 ++++++++++++++++++---------------- cmd/deploy_test.go | 121 +++++++++++---- cmd/run.go | 12 +- docs/reference/func_deploy.md | 5 +- e2e/e2e_remote_test.go | 7 +- 7 files changed, 276 insertions(+), 184 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index 245d9923be..f7f4e25822 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -153,16 +153,15 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro cfg buildConfig f fn.Function ) - cfg = newBuildConfig() - if f, err = fn.NewFunction(cfg.Path); err != nil { // Read in the Function - return - } - if cfg, err = cfg.Prompt(f); err != nil { + if cfg, err = newBuildConfig().Prompt(); err != nil { return wrapPromptError(err, "build") } if err = cfg.Validate(cmd); err != nil { // Perform any pre-validation return wrapValidateError(err, "build") } + if f, err = fn.NewFunction(cfg.Path); err != nil { // Read in the Function + return + } if !f.Initialized() { return NewErrNotInitializedFromPath(f.Root, "build") } @@ -301,9 +300,8 @@ func (c buildConfig) Configure(f fn.Function) fn.Function { // Prompt the user with value of config members, allowing for interactive changes. // Skipped if not in an interactive terminal (non-TTY), or if --confirm false (agree to -// all prompts) was set (default). f is the function being configured, which -// need not be on the local filesystem. -func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { +// all prompts) was set (default). +func (c buildConfig) Prompt() (buildConfig, error) { // If there is no registry nor explicit image name defined, the // Registry prompt is shown whether or not we are in confirm mode. // Otherwise, it is only shown if in confirm mode @@ -311,6 +309,10 @@ func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { // value and will always use the value from the config (flag or env variable). // This is not strictly correct and will be fixed when Global Config: Function // Context is available (PR#1416) + f, err := fn.NewFunction(c.Path) + if err != nil { + return c, err + } // Check if function exists first if !f.Initialized() { @@ -328,7 +330,7 @@ func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { err := survey.AskOne( &survey.Input{Message: "Registry for function images:", Default: c.Registry}, &c.Registry, - survey.WithValidator(NewRegistryValidator(f))) + survey.WithValidator(NewRegistryValidator(c.Path))) if err != nil { return c, fn.ErrRegistryRequired } @@ -351,6 +353,13 @@ func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { Message: "Optionally specify an exact image name to use (e.g. quay.io/boson/node-sample:latest)", }, }, + { + Name: "path", + Prompt: &survey.Input{ + Message: "Project path:", + Default: c.Path, + }, + }, { Name: "builder", Prompt: &survey.Select{ @@ -368,7 +377,7 @@ func (c buildConfig) Prompt(f fn.Function) (buildConfig, error) { }, } - err := survey.Ask(qs, &c) + err = survey.Ask(qs, &c) if err != nil { return c, err } diff --git a/cmd/config_git_set.go b/cmd/config_git_set.go index f6a64e89bc..77368853e4 100644 --- a/cmd/config_git_set.go +++ b/cmd/config_git_set.go @@ -155,7 +155,7 @@ func newConfigGitSetConfig(_ *cobra.Command) (c configGitSetConfig) { func (c configGitSetConfig) Prompt(f fn.Function) (configGitSetConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { return c, err } diff --git a/cmd/deploy.go b/cmd/deploy.go index ae3218d409..177af2553a 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "slices" "strconv" "strings" @@ -12,8 +13,10 @@ import ( "github.com/google/go-containerregistry/pkg/name" "github.com/ory/viper" "github.com/spf13/cobra" + "github.com/spf13/pflag" "k8s.io/apimachinery/pkg/api/resource" "knative.dev/client/pkg/util" + "knative.dev/func/cmd/common" "knative.dev/func/pkg/builders" "knative.dev/func/pkg/config" "knative.dev/func/pkg/deployers" @@ -86,7 +89,10 @@ DESCRIPTION The function is then read from the repository, so no local copy is needed, and nothing is written locally: to change the function, clone the repository, edit it and deploy the working tree. Choose the - directory within the repository with '--source-dir'. + directory within the repository with '--source-dir'. The function is + deployed as configured by the func.yaml committed there: of the flags + which configure a function, only '--registry' and '--registry-insecure' + apply. Domain When deploying, a function's route is automatically generated using the @@ -274,14 +280,23 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { // Initialize config first cfg = newDeployConfig(cmd) - // Load the function to deploy - if f, cfg, err = cfg.loadFunction(cmd); err != nil { + // A git repository deployed remotely needs no local function + if cfg.Remote && cfg.Source != "" { + return runDeployFromGit(cmd, cfg, newClient) + } + + // Create function object to check if initialized + if f, err = fn.NewFunction(cfg.Path); err != nil { return } - source := cfg.gitSource() + + // Check if function exists BEFORE prompting for config + if !f.Initialized() { + return NewErrNotInitializedFromPath(f.Root, "deploy") + } // Now that we know function exists, proceed with prompting - if cfg, err = cfg.Prompt(f); err != nil { + if cfg, err = cfg.Prompt(); err != nil { if errors.Is(err, fn.ErrRegistryRequired) { return NewErrRegistryRequired(err, "deploy") } @@ -290,13 +305,6 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { if err = cfg.Validate(cmd); err != nil { return wrapValidateError(err, "deploy") } - // The prompt may have chosen a git repository as the source, or another - // one than the function was read from. - if cfg.Remote && cfg.Source != "" && (f.Root != "" || cfg.gitSource() != source) { - if f, cfg, err = cfg.loadFunction(cmd); err != nil { - return - } - } // Warn if registry changed but registryInsecure is still true warnRegistryInsecureChange(cmd.OutOrStderr(), cfg.Registry, f) @@ -439,13 +447,6 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { } // Write - // A function deployed from a git repository is deployed by reference: - // nothing is written, neither to the repository nor to whatever is in the - // current directory. To change such a function, clone the repository, - // edit it and deploy the working tree. - if f.Root == "" { - return nil - } if err = f.Write(); err != nil { return } @@ -457,107 +458,6 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return f.Stamp() } -// loadFunction returns the function to deploy, and the config completed -// with the defaults that function provides. When a git repository is the -// source of a remote deployment it is the function committed there: the -// pipeline must describe what the cluster builds, and the current directory -// plays no part. Otherwise it is the function at the path, which must be -// initialized. -func (c deployConfig) loadFunction(cmd *cobra.Command) (fn.Function, deployConfig, error) { - if c.Remote && c.Source != "" { - f, err := fn.NewFunctionFromGit(cmd.Context(), c.gitSource()) - if err != nil { - return f, c, err - } - return f, c.withFunctionDefaults(cmd, f), nil - } - f, err := fn.NewFunction(c.Path) - if err != nil { - return f, c, err - } - if !f.Initialized() { - return f, c, NewErrNotInitializedFromPath(f.Root, "deploy") - } - return f, c, nil -} - -// withFunctionDefaults returns the config with the defaults it would have had -// if f were the function in the current directory when the command was -// built: the values NewDeployCmd registers as flag defaults from the function -// with context, for every flag the user did not set on the command line or in -// the environment. It is used when the function comes from a git repository, -// which is known only at run time, so that its func.yaml is honoured the same -// way a local one is. -func (c deployConfig) withFunctionDefaults(cmd *cobra.Command, f fn.Function) deployConfig { - global, err := config.NewDefault() - if err != nil { - fmt.Fprintf(cmd.OutOrStdout(), "error loading config at '%v'. %v\n", config.File(), err) - } - global = global.Apply(f) - - unset := func(flag string) bool { - _, env := os.LookupEnv("FUNC_" + strings.ToUpper(strings.ReplaceAll(flag, "-", "_"))) - return !cmd.Flags().Changed(flag) && !env - } - if unset("builder") { - c.Builder = global.Builder - } - if unset("deployer") { - c.Deployer = global.Deployer - } - if unset("registry") { - c.Registry = global.Registry - } - if unset("registry-insecure") { - c.RegistryInsecure = global.RegistryInsecure - } - if unset("builder-image") { - c.BuilderImage = f.Build.BuilderImages[c.Builder] - } - if unset("base-image") { - c.BaseImage = f.Build.BaseImage - } - if unset("image") { - c.Image = f.Image - } - if unset("domain") { - c.Domain = f.Domain - } - if unset("remote-storage-class") { - c.RemoteStorageClass = f.Build.RemoteStorageClass - } - if unset("pvc-size") { - c.PVCSize = f.Build.PVCSize - } - if unset("service-account") { - c.ServiceAccountName = f.Deploy.ServiceAccountName - } - if unset("image-pull-secret") { - c.ImagePullSecret = f.Deploy.ImagePullSecret - } - if unset("expose") { - c.Expose = f.Expose - } - if unset("namespace") { - c.Namespace = defaultNamespace(f, c.Verbose) - } - if unset("management-disabled") { - c.ManagementDisabled = f.Deploy.ManagementDisabled - } - return c -} - -// gitSource is the git repository to build from, as configured: the URL -// may carry the revision as a fragment (#), which then -// wins, as it does in Configure. -func (c deployConfig) gitSource() fn.Source { - g := fn.Source{URL: c.Source, Revision: c.Revision, Dir: c.SourceDir} - if parts := strings.SplitN(c.Source, "#", 2); len(parts) == 2 { - g.URL, g.Revision = parts[0], parts[1] - } - return g -} - // build determines if the function should be built based on given flag func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { if flag == "auto" { @@ -577,7 +477,7 @@ func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { return false, nil } -func NewRegistryValidator(f fn.Function) survey.Validator { +func NewRegistryValidator(path string) survey.Validator { return func(val interface{}) error { // if the value passed in is the zero value of the appropriate type @@ -585,10 +485,15 @@ func NewRegistryValidator(f fn.Function) survey.Validator { return fn.ErrRegistryRequired } + f, err := fn.NewFunction(path) + if err != nil { + return err + } + // Set the function's registry to that provided f.Registry = val.(string) - _, err := f.ImageName() //image can be derived without any error + _, err = f.ImageName() //image can be derived without any error if err != nil { return fmt.Errorf("invalid registry [%q]: %w", val.(string), err) } @@ -770,9 +675,9 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { // Configure basic members f.Domain = c.Domain f.Namespace = c.Namespace - commit := f.Build.Source.Commit // the commit a function read from git was read at - f.Build.Source = c.gitSource() - f.Build.Source.Commit = commit + f.Build.Source.URL = c.Source + f.Build.Source.Dir = c.SourceDir + f.Build.Source.Revision = c.Revision f.Build.RemoteStorageClass = c.RemoteStorageClass f.Deploy.ServiceAccountName = c.ServiceAccountName f.Deploy.ImagePullSecret = c.ImagePullSecret @@ -797,6 +702,15 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { if err != nil { return f, err } + + // .Revision + // TODO: the system should support specifying revision (refSpec) as a URL + // fragment ([#]) throughout, which, when implemented, removes + // the need for the below split into separate members: + if parts := strings.SplitN(c.Source, "#", 2); len(parts) == 2 { + f.Build.Source.URL = parts[0] + f.Build.Source.Revision = parts[1] + } return f, nil } @@ -817,9 +731,9 @@ func applyEnvs(current []fn.Env, args []string) (final []fn.Env, err error) { // Prompt the user with value of config members, allowing for interactive changes. // Skipped if not in an interactive terminal (non-TTY), or if --yes (agree to // all prompts) was explicitly set. -func (c deployConfig) Prompt(f fn.Function) (deployConfig, error) { +func (c deployConfig) Prompt() (deployConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { return c, err } @@ -1013,13 +927,35 @@ func printDeployMessages(out io.Writer, f fn.Function) { // current invocation is not remote. (providing Git attributes directly // via flags without --remote will error elsewhere). // - // A func.yaml may carry source settings (build.source), which then serve - // as the defaults of the --source flags. When building _locally_, the deploy - // command uses the local source code and ignores them. Since this might be - // confusing, a warning is issued below. + // When invoking a remote build with --remote, the --git-X arguments + // are persisted to the local function's source code such that the reference + // is retained. Subsequent runs of deploy then need not have these arguments + // present. + // + // However, when building _locally_ thereafter, the deploy command should + // prefer the local source code, ignoring the values for --source etc. + // Since this might be confusing, a warning is issued below that the local + // function source does include a reference to a git repository, but that it + // will be ignored in favor of the local source code since --remote was not + // specified. if !f.Local.Remote && (f.Build.Source.URL != "" || f.Build.Source.Revision != "" || f.Build.Source.Dir != "") { fmt.Fprintf(out, "Warning: source settings are only applicable when running with --remote. Local source code will be used.") } + + // Git Branch Mismatch + // ------------------- + // When doing a remote build with --revision, warn if the local branch + // doesn't match, as this can lead to confusion about which func.yaml is used. + if f.Local.Remote && f.Build.Source.URL != "" && f.Build.Source.Revision != "" { + // Doing a remote build, specified a git repository to pull from, and + // specified a reference within that remote. + currentBranch, err := common.DefaultCurrentBranch(f.Root) + if err != nil { + fmt.Fprintf(out, "Warning: unable to verify local and remote references match. %v\n", err) + } else if currentBranch != f.Build.Source.Revision { + fmt.Fprintf(out, "Warning: Local git branch '%s' does not match --revision '%s'. The local func.yaml will be used for function metadata (name, runtime, etc). Ensure your local branch matches the remote branch to avoid deployment issues.\n", currentBranch, f.Build.Source.Revision) + } + } } // isDigested checks that the given image reference has a digest. Invalid @@ -1044,3 +980,89 @@ func warnExposeIgnore(w io.Writer, expose, deployer string) { "support external exposure via this field.\n", expose) } } + +// gitFlags are the only flags a deployment from a git repository takes: +// those which locate the function, the registry, which depends on where it +// is deployed, and those which are not the function's settings. The function +// is otherwise deployed as configured by the func.yaml committed there. +var gitFlags = []string{"remote", "source", "revision", "source-dir", "registry", + "registry-insecure", "registry-authfile", "username", "password", "token", "verbose"} + +// runDeployFromGit deploys the function committed to a git repository with a +// remote pipeline, which builds the commit the function was read at. The +// function is read into memory and nothing is written locally: to change it, +// clone the repository, edit it and deploy the working tree. Settings the +// function leaves empty default as for any function, never to those of a +// function in the current directory. +func runDeployFromGit(cmd *cobra.Command, cfg deployConfig, newClient ClientFactory) (err error) { + given := func(flag string) bool { + env := "FUNC_" + strings.ToUpper(strings.ReplaceAll(flag, "-", "_")) + return cmd.Flags().Changed(flag) || os.Getenv(env) != "" + } + var extra []string + cmd.Flags().VisitAll(func(fl *pflag.Flag) { + if given(fl.Name) && !slices.Contains(gitFlags, fl.Name) { + extra = append(extra, "--"+fl.Name) + } + }) + if len(extra) > 0 { + return fmt.Errorf("%s cannot be used with --source: the function is deployed as "+ + "configured by the func.yaml in the repository. To change it, clone the "+ + "repository, edit it and deploy the working tree", strings.Join(extra, ", ")) + } + + // The revision may also be given as the URL's fragment (#), + // which then wins, as it does in Configure. + source := fn.Source{URL: cfg.Source, Revision: cfg.Revision, Dir: cfg.SourceDir} + if parts := strings.SplitN(cfg.Source, "#", 2); len(parts) == 2 { + source.URL, source.Revision = parts[0], parts[1] + } + f, err := fn.NewFunctionFromGit(cmd.Context(), source) + if err != nil { + return + } + + global, err := config.NewDefault() + if err != nil { + return + } + if f.Build.Builder == "" { + f.Build.Builder = global.Builder + } + if given("registry") { + f.Registry = cfg.Registry + } + if f.Registry == "" { + f.Registry = global.Registry + } + if given("registry-insecure") { + f.RegistryInsecure = cfg.RegistryInsecure + } + f.Namespace = defaultNamespace(f, cfg.Verbose) + if err = f.Validate(); err != nil { + return + } + + // The client is configured by cfg, whose defaults came from the current + // directory. + cfg.Builder, cfg.Registry, cfg.RegistryInsecure = f.Build.Builder, f.Registry, f.RegistryInsecure + clientOptions, err := cfg.clientOptions() + if err != nil { + return + } + client, done := newClient(ClientConfig{Verbose: cfg.Verbose, InsecureSkipVerify: cfg.RegistryInsecure}, clientOptions...) + defer done() + + url, f, err := client.RunPipeline(cmd.Context(), f) + if errors.Is(err, fn.ErrRegistryRequired) { + return NewErrRegistryRequired(err, "deploy") + } else if err != nil { + return wrapDeploymentError(err) + } + fmt.Fprintf(cmd.OutOrStdout(), "Function Deployed at %v\n", url) + if fn.ExposureRecordMissing(f.Expose, f.Deploy.Expose, f.Deploy.Deployer) { + fmt.Fprintf(cmd.OutOrStderr(), "Warning: expose %q was requested but the cluster's "+ + "func-util image applied no external exposure; the function is running cluster-local\n", f.Expose) + } + return nil +} diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index a8af317903..69f4e27e22 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "strings" "testing" "time" @@ -15,6 +16,7 @@ import ( "github.com/AlecAivazis/survey/v2/core" "github.com/ory/viper" "github.com/spf13/cobra" + "github.com/spf13/pflag" "knative.dev/func/pkg/builders" "knative.dev/func/pkg/config" @@ -582,14 +584,13 @@ func TestDeploy_RemoteGitNoLocalFunction(t *testing.T) { cmd.SetArgs([]string{"--remote", "--source=" + url, "--revision=feature", - "--source-dir=functions/remote-fn", - "--namespace=fnns"}) + "--source-dir=functions/remote-fn"}) if err := cmd.Execute(); err != nil { t.Fatal(err) } - // The pipeline received the function from the repository, configured - // by the flags, and builds the commit it was read at + // The pipeline received the function from the repository and builds the + // commit it was read at want := fn.Source{URL: url, Revision: "feature", Dir: "functions/remote-fn", Commit: heads["feature"]} if !pipeliner.RunInvoked { t.Fatal("pipeline was not invoked") @@ -600,8 +601,8 @@ func TestDeploy_RemoteGitNoLocalFunction(t *testing.T) { if deployed.Root != "" { t.Errorf("expected no root, got %q", deployed.Root) } - if deployed.Namespace != "fnns" || deployed.Build.Source != want { - t.Errorf("expected flags to configure the deployed function, got %+v", deployed) + if deployed.Build.Source != want { + t.Errorf("expected source %+v, got %+v", want, deployed.Build.Source) } // Nothing was written locally if _, err := os.Stat(filepath.Join(root, fn.FunctionFile)); !os.IsNotExist(err) { @@ -644,7 +645,7 @@ func TestDeploy_RemoteGitWritesNothing(t *testing.T) { fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry), )) - cmd.SetArgs([]string{"--remote", "--source=" + url, "--namespace=fnns"}) + cmd.SetArgs([]string{"--remote", "--source=" + url}) if err := cmd.Execute(); err != nil { t.Fatal(err) } @@ -668,22 +669,31 @@ func TestDeploy_RemoteGitWritesNothing(t *testing.T) { } } -// TestDeploy_RemoteGitDefaultsFromRepository ensures the flag defaults come -// from the repository's func.yaml, as they come from a local func.yaml, with -// flags and environment variables still taking precedence. The command's -// flag defaults were derived from the (here empty) current directory. +// TestDeploy_RemoteGitDefaultsFromRepository ensures the function is deployed +// as configured by the repository's func.yaml, except for the registry (and +// whether it is insecure) given as a flag or environment variable, and that +// the settings it leaves empty default as for any function. The function in +// the current directory, from which the command's flag defaults were derived, +// plays no part. func TestDeploy_RemoteGitDefaultsFromRepository(t *testing.T) { - _ = FromTempDirectory(t) + root := FromTempDirectory(t) url, _ := ServeGitRepository(t, map[string]map[string]string{ "main": {"func.yaml": funcYAML("remote-fn", `registry: example.com/repo -deployer: raw build: builder: s2i + pvcSize: 2Gi deploy: namespace: repo-ns - serviceAccountName: repo-sa `)}, + "bare": {"func.yaml": funcYAML("bare-fn", "")}, }) + if _, err := fn.New().Init(fn.Function{Name: "local-fn", Runtime: "go", Root: root, + Registry: "example.com/local", + Namespace: "local-ns", + Build: fn.BuildSpec{Builder: "s2i", PVCSize: "5Gi"}, + }); err != nil { + t.Fatal(err) + } deploy := func(t *testing.T, args ...string) fn.Function { t.Helper() @@ -702,23 +712,68 @@ deploy: return deployed } - // Nothing set: the repository's values apply - f := deploy(t) - if f.Build.Builder != "s2i" || f.Registry != "example.com/repo" || f.Deployer != "raw" || - f.Deploy.ServiceAccountName != "repo-sa" || f.Namespace != "repo-ns" { - t.Errorf("expected the repository's func.yaml to provide the defaults, got builder=%q registry=%q deployer=%q sa=%q namespace=%q", - f.Build.Builder, f.Registry, f.Deployer, f.Deploy.ServiceAccountName, f.Namespace) + // The repository's values apply + f := deploy(t, "--revision=main") + if f.Name != "remote-fn" || f.Build.Builder != "s2i" || f.Registry != "example.com/repo" || + f.Build.PVCSize != "2Gi" || f.Namespace != "repo-ns" { + t.Errorf("expected the repository's func.yaml to configure the function, got name=%q builder=%q registry=%q pvc=%q namespace=%q", + f.Name, f.Build.Builder, f.Registry, f.Build.PVCSize, f.Namespace) } - // Flags and environment variables win over the repository - t.Setenv("FUNC_DEPLOYER", "knative") - f = deploy(t, "--builder=pack", "--registry=example.com/flag", "--namespace=flag-ns") - if f.Build.Builder != "pack" || f.Registry != "example.com/flag" || f.Deployer != "knative" || f.Namespace != "flag-ns" { - t.Errorf("expected flags and env to win, got builder=%q registry=%q deployer=%q namespace=%q", - f.Build.Builder, f.Registry, f.Deployer, f.Namespace) + // Empty settings default as for any function, not to the local function's + f = deploy(t, "--revision=bare") + if f.Build.Builder != builders.Default || f.Build.PVCSize != "" || f.Registry == "example.com/local" || f.Namespace == "local-ns" { + t.Errorf("expected the defaults of a function, got builder=%q pvc=%q registry=%q namespace=%q", + f.Build.Builder, f.Build.PVCSize, f.Registry, f.Namespace) + } + + // The registry may be given as an environment variable or a flag + t.Setenv("FUNC_REGISTRY", "example.com/env") + if f = deploy(t, "--revision=main"); f.Registry != "example.com/env" { + t.Errorf("expected the registry from the environment, got %q", f.Registry) } - if f.Deploy.ServiceAccountName != "repo-sa" { - t.Errorf("expected the untouched service account to stay the repository's, got %q", f.Deploy.ServiceAccountName) + if f = deploy(t, "--revision=main", "--registry=example.com/flag"); f.Registry != "example.com/flag" { + t.Errorf("expected the registry from the flag, got %q", f.Registry) + } + if f = deploy(t, "--revision=main", "--registry-insecure"); !f.RegistryInsecure { + t.Error("expected the registry to be insecure, as the flag says") + } +} + +// TestDeploy_RemoteGitRejectsFlags ensures that a deployment from a git +// repository takes none of the flags which configure the function, whether +// given as a flag or an environment variable: the function is deployed as +// committed, so they fail the deployment instead of being dropped. +func TestDeploy_RemoteGitRejectsFlags(t *testing.T) { + _ = FromTempDirectory(t) + url := serveFunction(t, "remote-fn") + + var rejected []string + NewDeployCmd(NewTestClient()).Flags().VisitAll(func(fl *pflag.Flag) { + if !slices.Contains(gitFlags, fl.Name) { + rejected = append(rejected, fl.Name) + } + }) + for _, flag := range rejected { + t.Run(flag, func(t *testing.T) { + pipeliner := mock.NewPipelinesProvider() + cmd := NewDeployCmd(NewTestClient(fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry))) + cmd.SetArgs([]string{"--remote", "--source=" + url, "--" + flag + "=true"}) + err := cmd.Execute() + if err == nil || !strings.Contains(err.Error(), "--"+flag+" cannot be used with --source") { + t.Fatalf("expected --%s to be rejected, got %v", flag, err) + } + if pipeliner.RunInvoked { + t.Error("pipeline should not run") + } + }) + } + + t.Setenv("FUNC_NAMESPACE", "env-ns") + cmd := NewDeployCmd(NewTestClient(fn.WithPipelinesProvider(mock.NewPipelinesProvider()), fn.WithRegistry(TestRegistry))) + cmd.SetArgs([]string{"--remote", "--source=" + url}) + if err := cmd.Execute(); err == nil || !strings.Contains(err.Error(), "--namespace cannot be used with --source") { + t.Fatalf("expected FUNC_NAMESPACE to be rejected, got %v", err) } } @@ -859,7 +914,7 @@ func TestDeploy_RemoteSourceFragment(t *testing.T) { fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry), )) - cmd.SetArgs([]string{"--remote", "--source=" + url, "--namespace=fnns"}) + cmd.SetArgs([]string{"--remote", "--source=" + url}) if err := cmd.Execute(); err != nil { t.Fatal(err) @@ -1879,6 +1934,14 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) { // Assertions if remote != "" && remote != "false" { // the default of "" is == false + // A function deployed from git takes no --build + if url != "" && build != "" { + if err == nil || !strings.Contains(err.Error(), "--build cannot be used with --source") { + t.Fatalf("expected --build to be rejected, got %v", err) + } + return + } + // REMOTE Assertions if !pipeliner.RunInvoked { // Remote deployer should be triggered t.Error("remote was not invoked") diff --git a/cmd/run.go b/cmd/run.go index c8ac5ee8ac..f60ef3e9aa 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -154,13 +154,13 @@ func runRun(cmd *cobra.Command, newClient ClientFactory) (err error) { cfg runConfig f fn.Function ) - cfg = newRunConfig(cmd) + if cfg, err = newRunConfig(cmd).Prompt(); err != nil { + return wrapPromptError(err, "run") + } + if f, err = fn.NewFunction(cfg.Path); err != nil { return } - if cfg, err = cfg.Prompt(f); err != nil { - return wrapPromptError(err, "run") - } if !f.Initialized() { return NewErrNotInitializedFromPath(f.Root, "run") } @@ -371,10 +371,10 @@ func (c runConfig) Configure(f fn.Function) (fn.Function, error) { return f, err } -func (c runConfig) Prompt(f fn.Function) (runConfig, error) { +func (c runConfig) Prompt() (runConfig, error) { var err error - if c.buildConfig, err = c.buildConfig.Prompt(f); err != nil { + if c.buildConfig, err = c.buildConfig.Prompt(); err != nil { return c, err } diff --git a/docs/reference/func_deploy.md b/docs/reference/func_deploy.md index 01e43a03dd..2fd8b0e5c5 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -62,7 +62,10 @@ DESCRIPTION The function is then read from the repository, so no local copy is needed, and nothing is written locally: to change the function, clone the repository, edit it and deploy the working tree. Choose the - directory within the repository with '--source-dir'. + directory within the repository with '--source-dir'. The function is + deployed as configured by the func.yaml committed there: of the flags + which configure a function, only '--registry' and '--registry-insecure' + apply. Domain When deploying, a function's route is automatically generated using the diff --git a/e2e/e2e_remote_test.go b/e2e/e2e_remote_test.go index 45467bd19c..e36b4c2215 100644 --- a/e2e/e2e_remote_test.go +++ b/e2e/e2e_remote_test.go @@ -43,7 +43,7 @@ func TestRemote_Deploy(t *testing.T) { // TestRemote_Source ensures a remote build can be triggered which pulls // source from a remote repository, with no local copy of the function. // -// func deploy --remote --source={url} --registry={} --builder=pack +// func deploy --remote --source={url} --registry={} func TestRemote_Source(t *testing.T) { name := "func-e2e-test-remote-source" _ = fromCleanEnv(t, name) @@ -53,7 +53,6 @@ func TestRemote_Source(t *testing.T) { if err := newCmd(t, "deploy", "--remote", "--source", "https://github.com/functions-dev/func-e2e-tests", "--registry", Registry, - "--builder", "pack", ).Run(); err != nil { t.Fatal(err) } @@ -81,8 +80,6 @@ func TestRemote_Ref(t *testing.T) { "--source", "https://github.com/functions-dev/func-e2e-tests", "--revision", name, "--registry", Registry, - "--builder", "pack", - "--build", ).Run(); err != nil { t.Fatal(err) } @@ -110,8 +107,6 @@ func TestRemote_Dir(t *testing.T) { "--source", "https://github.com/functions-dev/func-e2e-tests", "--source-dir", name, "--registry", Registry, - "--builder", "pack", - "--build", ).Run(); err != nil { t.Fatal(err) }