From dcb41572574341458bee7ed606a896a77920786d Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 2 Sep 2026 21:45:10 +0200 Subject: [PATCH 1/6] fix: wait for the coredns rollout in func cluster create Magic DNS patched the coredns deployment, slept, then waited for every kube-system pod to be Ready. The old coredns pods are terminating after the patch and never become Ready again, so the wait timed out after 60s on hosts where they took a while to go, and the whole cluster was torn down at the very last step. Wait for the deployment rollout instead: the new pods are available and the old ones are gone. hack/cluster.sh has the same sleep-and-wait, from which this was ported. --- pkg/cluster/dns.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/pkg/cluster/dns.go b/pkg/cluster/dns.go index 7a0781df36..25cf1fdaa6 100644 --- a/pkg/cluster/dns.go +++ b/pkg/cluster/dns.go @@ -27,17 +27,14 @@ func configureMagicDNS(ctx context.Context, cfg ClusterConfig, out io.Writer) er return err } - // Deployment patch triggers a rolling restart. Sleep so the new pods - // enter NotReady before we wait for Ready — otherwise the old pods - // still satisfy the condition and we return before the restart lands. - if err := wait(ctx, 5*time.Second); err != nil { - return err - } + // The deployment patch triggers a rolling restart. Wait for the rollout + // itself: the new pods are available and the old ones are gone. Waiting + // for every kube-system pod to be Ready instead raced with the old + // coredns pods, which are terminating and never become Ready again. if err := run(ctx, out, "", - cfg.kubectl(), "wait", "pod", - "--for=condition=Ready", "-l", "!job-name", - "-n", "kube-system", "--timeout=60s"); err != nil { - return fmt.Errorf("waiting for coredns: %w", err) + cfg.kubectl(), "rollout", "status", "deployment/coredns", + "-n", "kube-system", "--timeout=120s"); err != nil { + return fmt.Errorf("waiting for coredns rollout: %w", err) } success(out, "Magic DNS", time.Since(start)) From a84a7e29c7aa3388356797b329d0d193374346de Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 2 Sep 2026 20:49:56 +0200 Subject: [PATCH 2/6] 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 GitRemoteCommit reports the commit that revision resolves to. Both reuse 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. Groundwork for knative/func#3203. --- pkg/functions/client.go | 8 +- pkg/functions/function.go | 50 ++++--- pkg/functions/function_git.go | 200 +++++++++++++++++++++++++++ pkg/functions/function_git_test.go | 196 ++++++++++++++++++++++++++ pkg/functions/function_migrations.go | 115 ++++++++------- pkg/functions/git_commit.go | 17 +++ 6 files changed, 502 insertions(+), 84 deletions(-) create mode 100644 pkg/functions/function_git_test.go 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 df1762350d..fd6d140eff 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -374,24 +374,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 { @@ -404,6 +390,33 @@ func NewFunction(root string) (f Function, err error) { return } +// 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. func (f Function) Validate() error { @@ -680,7 +693,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 index b3122ac791..47f02b80d1 100644 --- a/pkg/functions/function_git.go +++ b/pkg/functions/function_git.go @@ -1,10 +1,22 @@ package functions import ( + "context" + "errors" "fmt" + "os" + "path" "strings" giturls "github.com/chainguard-dev/git-urls" + "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" ) type Git struct { @@ -32,3 +44,191 @@ func validateGit(git Git) (errors []string) { } return } + +// NewFunctionFromGit loads the function committed in the repository g +// describes: the func.yaml in g.ContextDir 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). +func NewFunctionFromGit(ctx context.Context, g Git) (Function, error) { + src, err := resolveGitSource(ctx, g) + if err != nil { + return Function{}, err + } + tree, err := src.checkout(ctx) + if err != nil { + return Function{}, err + } + bb, err := util.ReadFile(tree, path.Join(g.ContextDir, FunctionFile)) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return Function{}, fmt.Errorf("no %s in %q of %s at %s", FunctionFile, g.ContextDir, g.URL, src.describe()) + } + return Function{}, fmt.Errorf("cannot read %s from %s: %w", FunctionFile, g.URL, err) + } + return parseFunction(bb) +} + +// gitSource is a revision of a remote repository, resolved against the refs +// the remote advertises. +type gitSource struct { + url string + auth transport.AuthMethod + // ref is the branch or tag to fetch. It is empty when the revision was + // given as a bare commit hash. + ref plumbing.ReferenceName + // hash is the commit the revision resolves to. + hash plumbing.Hash +} + +// resolveGitSource lists the refs of g.URL and matches g.Revision against +// them. An empty revision means the remote's default branch. Otherwise a +// branch, a tag, a full ref name and a full commit hash are tried, in that +// order. +func resolveGitSource(ctx context.Context, g Git) (gitSource, error) { + src := gitSource{url: g.URL} + if g.URL == "" { + return src, errors.New("git URL required") + } + + remote := git.NewRemote(memory.NewStorage(), &config.RemoteConfig{ + Name: git.DefaultRemoteName, + URLs: []string{g.URL}, + }) + opts := &git.ListOptions{PeelingOption: git.AppendPeeled} + refs, err := remote.ListContext(ctx, opts) + if isAuthError(err) { + if src.auth = credentialsForURL(g.URL); src.auth != nil { + opts.Auth = src.auth + refs, err = remote.ListContext(ctx, opts) + } + } + if err != nil { + return src, fmt.Errorf("cannot list refs of %s: %w", g.URL, err) + } + + byName := make(map[plumbing.ReferenceName]*plumbing.Reference, len(refs)) + for _, r := range refs { + byName[r.Name()] = r + } + // commitOf returns the commit a ref points to: the peeled object for an + // annotated tag, the ref's own hash otherwise. + commitOf := func(name plumbing.ReferenceName) plumbing.Hash { + if peeled, ok := byName[name+"^{}"]; ok { + return peeled.Hash() + } + return byName[name].Hash() + } + + if g.Revision == "" { + head, ok := byName[plumbing.HEAD] + if !ok { + return src, fmt.Errorf("%s advertises no HEAD; specify a revision", g.URL) + } + if head.Type() != plumbing.SymbolicReference { + src.hash = head.Hash() + return src, nil + } + if _, ok := byName[head.Target()]; !ok { + return src, fmt.Errorf("%s: HEAD points to %s, which the remote does not advertise", g.URL, head.Target()) + } + src.ref = head.Target() + src.hash = commitOf(src.ref) + return src, nil + } + + for _, name := range []plumbing.ReferenceName{ + plumbing.NewBranchReferenceName(g.Revision), + plumbing.NewTagReferenceName(g.Revision), + plumbing.ReferenceName(g.Revision), + } { + if _, ok := byName[name]; ok { + src.ref = name + src.hash = commitOf(name) + return src, nil + } + } + if plumbing.IsHash(g.Revision) { + src.hash = plumbing.NewHash(g.Revision) + return src, nil + } + return src, fmt.Errorf("revision %q not found in %s", g.Revision, g.URL) +} + +// describe returns the revision for messages: the ref's short name when the +// revision named one, the commit hash otherwise. +func (s gitSource) describe() string { + if s.ref != "" { + return s.ref.Short() + } + return s.hash.String() +} + +// checkout fetches the resolved revision, depth one, into memory and returns +// its tree. +func (s gitSource) checkout(ctx context.Context) (billy.Filesystem, error) { + var ( + repo *git.Repository + err error + ) + if s.ref != "" { + repo, err = git.CloneContext(ctx, memory.NewStorage(), memfs.New(), &git.CloneOptions{ + URL: s.url, + Auth: s.auth, + ReferenceName: s.ref, + SingleBranch: true, + Depth: 1, + Tags: git.NoTags, + RecurseSubmodules: git.NoRecurseSubmodules, + }) + } else { + // A bare commit cannot be cloned: fetch it by hash and check it out. + repo, err = fetchGitCommit(ctx, s) + } + if err != nil { + return nil, fmt.Errorf("cannot fetch %s at %s: %w", s.url, s.describe(), err) + } + wt, err := repo.Worktree() + if err != nil { + return nil, err + } + return wt.Filesystem, nil +} + +func fetchGitCommit(ctx context.Context, s gitSource) (*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{s.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: s.auth, + Depth: 1, + Tags: git.NoTags, + RefSpecs: []config.RefSpec{ + config.RefSpec(s.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: s.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..d7a910426b --- /dev/null +++ b/pkg/functions/function_git_test.go @@ -0,0 +1,196 @@ +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 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("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.Git + wantName string + }{ + {"default branch", fn.Git{URL: fx.url}, "root-fn"}, + {"context dir", fn.Git{URL: fx.url, ContextDir: "sub"}, "sub-fn"}, + {"branch", fn.Git{URL: fx.url, Revision: "feature"}, "feature-fn"}, + {"annotated tag", fn.Git{URL: fx.url, Revision: "v1"}, "root-fn"}, + {"full ref", fn.Git{URL: fx.url, Revision: "refs/heads/feature"}, "feature-fn"}, + {"commit hash", fn.Git{URL: fx.url, Revision: fx.feature}, "feature-fn"}, + } + 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.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.Git{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) + } + // Moved from the top level into build.git by migrateToSpecsStructure + if f.Build.Git.URL != "http://test-url" { + t.Errorf("expected migrated git url, got %q", f.Build.Git.URL) + } +} + +// 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.Git + wantErr string + }{ + {"unknown revision", fn.Git{URL: fx.url, Revision: "nope"}, "not found"}, + {"missing func.yaml", fn.Git{URL: fx.url, ContextDir: "nope"}, "no func.yaml"}, + {"no url", fn.Git{}, "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) + } + }) + } +} + +// TestGitRemoteCommit ensures the commit reported for a remote revision is +// the commit that revision resolves to: an annotated tag yields the tagged +// commit, not the tag object. +func TestGitRemoteCommit(t *testing.T) { + fx := newGitFixture(t) + tests := []struct { + name string + git fn.Git + want string + }{ + {"default branch", fn.Git{URL: fx.url}, fx.main[:7]}, + {"branch", fn.Git{URL: fx.url, Revision: "feature"}, fx.feature[:7]}, + {"annotated tag", fn.Git{URL: fx.url, Revision: "v1"}, fx.main[:7]}, + {"commit hash", fn.Git{URL: fx.url, Revision: fx.feature}, fx.feature[:7]}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := fn.GitRemoteCommit(context.Background(), tt.git) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Errorf("expected commit %q, got %q", tt.want, got) + } + }) + } +} diff --git a/pkg/functions/function_migrations.go b/pkg/functions/function_migrations.go index 42995cc59c..d9758f9d9d 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). @@ -125,7 +151,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 @@ -172,16 +198,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 @@ -205,18 +226,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 @@ -226,16 +240,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 != "" { @@ -305,16 +314,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" { @@ -326,13 +330,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 { @@ -340,11 +338,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 { diff --git a/pkg/functions/git_commit.go b/pkg/functions/git_commit.go index d4ecdb351d..e89cd0f655 100644 --- a/pkg/functions/git_commit.go +++ b/pkg/functions/git_commit.go @@ -1,6 +1,8 @@ package functions import ( + "context" + "github.com/go-git/go-git/v5" ) @@ -39,3 +41,18 @@ func GitCommit(dir string) (string, error) { return sha, nil } + +// GitRemoteCommit returns the short commit SHA that g.Revision of the +// repository at g.URL currently resolves to, without a local checkout. It is +// the remote counterpart of GitCommit, so a build from a git repository can +// be labelled with the commit that is actually built. +func GitRemoteCommit(ctx context.Context, g Git) (string, error) { + src, err := resolveGitSource(ctx, g) + if err != nil { + return "", err + } + if src.hash.IsZero() { + return "", nil + } + return src.hash.String()[:7], nil +} From 64e55d29d7134adbb23ce544f508553053aa4c67 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 2 Sep 2026 20:52:38 +0200 Subject: [PATCH 3/6] fix: label remote builds with the commit they build The Tekton provider read two things from the local checkout that do not describe a build from a git repository: the commit label came from the local HEAD (fn.GitCommit on f.Root) rather than the revision the cluster clones, and a .tekton/ override was looked up under f.Root, which for a function without a Root resolved to the current directory. The commit is now resolved from the source the pipeline builds, via GitRemoteCommit for a git source, and before any cluster resource is created, so an unknown revision fails early. The override lookup is skipped without a Root, and uploading sources without a Root is refused with a clear error. --- pkg/pipelines/tekton/pipelines_provider.go | 30 +++++++- pkg/pipelines/tekton/source_commit_test.go | 89 ++++++++++++++++++++++ pkg/pipelines/tekton/templates.go | 15 ++-- pkg/pipelines/tekton/templates_test.go | 31 +++++++- 4 files changed, 156 insertions(+), 9 deletions(-) create mode 100644 pkg/pipelines/tekton/source_commit_test.go diff --git a/pkg/pipelines/tekton/pipelines_provider.go b/pkg/pipelines/tekton/pipelines_provider.go index bba07e4da2..5abad58a0a 100644 --- a/pkg/pipelines/tekton/pipelines_provider.go +++ b/pkg/pipelines/tekton/pipelines_provider.go @@ -123,6 +123,19 @@ 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.Git.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") + } + + // Resolve the commit to label the image with before creating any cluster + // resources: an unreachable repository or unknown revision fails here. + commit, err := sourceCommit(ctx, f) + if err != nil { + return "", f, err + } + // 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. @@ -239,7 +252,7 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return "", f, fmt.Errorf("problem in creating secret: %v", err) } - err = createAndApplyPipelineRunTemplate(f, namespace, labels) + err = createAndApplyPipelineRunTemplate(f, namespace, labels, commit) if err != nil { return "", f, fmt.Errorf("problem in creating pipeline run: %v", err) } @@ -305,6 +318,21 @@ func (pp *PipelinesProvider) Run(ctx context.Context, f fn.Function) (string, fn return obj.Route, f, nil } +// sourceCommit returns the short commit SHA of the source the pipeline +// builds, used to label the image (org.opencontainers.image.revision): the +// resolved revision of the git repository when one is set, the local +// checkout otherwise. +func sourceCommit(ctx context.Context, f fn.Function) (string, error) { + if f.Build.Git.URL != "" { + commit, err := fn.GitRemoteCommit(ctx, f.Build.Git) + if err != nil { + return "", fmt.Errorf("cannot resolve the git source: %w", err) + } + return commit, nil + } + return fn.GitCommit(f.Root) +} + // Creates tar stream with the function sources as they were in "./source" directory. func sourcesAsTarStream(f fn.Function) *io.PipeReader { // Apply the same ignore policy as local builds: fn.IsIgnored always excludes diff --git a/pkg/pipelines/tekton/source_commit_test.go b/pkg/pipelines/tekton/source_commit_test.go new file mode 100644 index 0000000000..59e602be41 --- /dev/null +++ b/pkg/pipelines/tekton/source_commit_test.go @@ -0,0 +1,89 @@ +package tekton + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + fn "knative.dev/func/pkg/functions" +) + +// newGitRepo creates a repository with a single commit of a func.yaml and +// returns its path and the full hash of that commit. +func newGitRepo(t *testing.T) (dir, commit string) { + 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)) + } + content := "specVersion: " + fn.LastSpecVersion() + "\nname: f\nruntime: go\ncreated: 2024-01-01T00:00:00Z\n" + if err := os.WriteFile(filepath.Join(dir, "func.yaml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + run("init", "-q", "-b", "main") + run("add", ".") + run("commit", "-q", "-m", "initial") + return dir, run("rev-parse", "HEAD") +} + +// Test_sourceCommit ensures the image is labelled with the commit of the +// source the pipeline builds: the git revision when a repository is set, +// even from within an unrelated local checkout, and the local checkout +// otherwise. +func Test_sourceCommit(t *testing.T) { + remoteDir, remoteCommit := newGitRepo(t) + localDir, localCommit := newGitRepo(t) + + tests := []struct { + name string + f fn.Function + want string + }{ + {"git source without a local checkout", + fn.Function{Build: fn.BuildSpec{Git: fn.Git{URL: "file://" + remoteDir}}}, + remoteCommit[:7]}, + {"git source wins over a local checkout", + fn.Function{Root: localDir, Build: fn.BuildSpec{Git: fn.Git{URL: "file://" + remoteDir}}}, + remoteCommit[:7]}, + {"local checkout", + fn.Function{Root: localDir}, + localCommit[:7]}, + {"no source information", + fn.Function{Root: t.TempDir()}, + ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := sourceCommit(context.Background(), tt.f) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Errorf("expected commit %q, got %q", tt.want, got) + } + }) + } + + t.Run("unknown revision is an error", func(t *testing.T) { + f := fn.Function{Build: fn.BuildSpec{Git: fn.Git{URL: "file://" + remoteDir, Revision: "nope"}}} + if _, err := sourceCommit(context.Background(), f); err == nil { + t.Fatal("expected an error") + } + }) +} diff --git a/pkg/pipelines/tekton/templates.go b/pkg/pipelines/tekton/templates.go index b853194bcf..eea8e20bc8 100644 --- a/pkg/pipelines/tekton/templates.go +++ b/pkg/pipelines/tekton/templates.go @@ -347,9 +347,11 @@ func createAndApplyPipelineTemplate(f fn.Function, namespace string, labels map[ return createAndApplyResource(f.Root, pipelineFileName, template, "pipeline", getPipelineName(f), namespace, data) } -// 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 { +// 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. +// commit is the source commit the image is labelled with (see sourceCommit). +func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels map[string]string, commit string) error { contextDir := f.Build.Git.ContextDir if contextDir == "" && f.Build.Builder == builders.S2I { // TODO(lkingland): could instead update S2I to interpret empty string @@ -389,8 +391,6 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m tlsVerify = "false" } - commit, _ := fn.GitCommit(f.Root) - data := templateData{ FunctionName: f.Name, Annotations: f.Deploy.Annotations, @@ -431,12 +431,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 7523ce931c..6e906482b8 100644 --- a/pkg/pipelines/tekton/templates_test.go +++ b/pkg/pipelines/tekton/templates_test.go @@ -297,6 +297,35 @@ var testData = []struct { }, } +// Test_createAndApplyPipelineRunTemplate_NoRoot ensures a function loaded +// from a git repository, which has no Root, yields 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, + Git: fn.Git{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, "abc1234"); err != nil { + t.Fatal(err) + } +} + func Test_createAndApplyPipelineRunTemplate(t *testing.T) { for _, tt := range testData { t.Run(tt.name, func(t *testing.T) { @@ -321,7 +350,7 @@ func Test_createAndApplyPipelineRunTemplate(t *testing.T) { f.Image = "docker.io/alice/" + f.Name f.Registry = TestRegistry - if err := createAndApplyPipelineRunTemplate(f, tt.namespace, tt.labels); (err != nil) != tt.wantErr { + if err := createAndApplyPipelineRunTemplate(f, tt.namespace, tt.labels, "abc1234"); (err != nil) != tt.wantErr { t.Errorf("createAndApplyPipelineRunTemplate() error = %v, wantErr %v", err, tt.wantErr) } }) From d1dd46f100273fc340a2c0b669231c8c1a21cde3 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 2 Sep 2026 20:56:46 +0200 Subject: [PATCH 4/6] feat: deploy --remote --git-url no longer needs a local checkout A remote deployment of a git repository had two sources of truth: the CLI built the Tekton pipeline from the local func.yaml while the cluster cloned the repository and read its func.yaml. The local copy existed only to feed the CLI, so it was required, and had to be on the right branch and in the right directory to match what the cluster built. The repository is now the source of the function when --remote has a git URL. The CLI reads func.yaml from the requested revision and directory (NewFunctionFromGit), applies the flags to that, and runs the pipeline for it. A local function at the path is optional. When present it records the request and the outcome (git settings, registry, deployed image, namespace, deployer, exposure) so describe, delete and later deploys find them, and its own metadata is left untouched. The prompts of build, run and deploy take the loaded function instead of loading one from the path themselves, and Validate no longer demands a Root: where a function lives is not part of its correctness, and Write is what needs one. The branch-mismatch warning is gone with the mismatch. Closes knative/func#3203. --- cmd/build.go | 21 ++--- cmd/config_git_set.go | 2 +- cmd/deploy.go | 131 +++++++++++++++------------ cmd/deploy_test.go | 165 +++++++++++++++++++++++++++++++++- cmd/run.go | 13 +-- docs/reference/func_deploy.md | 3 + pkg/functions/function.go | 11 +-- 7 files changed, 269 insertions(+), 77 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index f7f4e25822..7a958845b9 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -153,13 +153,17 @@ 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 + if f, err = fn.NewFunction(cfg.Path); err != nil { // The prompt may change the path return } if !f.Initialized() { @@ -300,8 +304,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 +314,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 +331,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 } @@ -377,7 +378,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 9b5998e595..6523d286b5 100644 --- a/cmd/config_git_set.go +++ b/cmd/config_git_set.go @@ -148,7 +148,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 b8b8273efd..2b0d5144c4 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,9 @@ DESCRIPTION eliminating the need for a local container engine. To trigger deployment of a git repository instead of local source, combine with '--git-url': '{{rootCmdUse}} deploy --remote --git-url=git.example.com/alice/f.git' + The function is then read from the repository, so no local copy is + needed. Choose the revision with '--git-branch' and the directory within + the repository with '--git-dir'. Domain When deploying, a function's route is automatically generated using the @@ -261,34 +264,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.Git.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 +291,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.GitURL != "" && 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 +439,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 +468,40 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) { return f.Stamp() } +// newFunctionFromGit loads a function from a git repository. A variable so +// tests can stand in for the network. +var newFunctionFromGit = fn.NewFunctionFromGit + +// 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.GitURL != "" { + return 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 takes precedence +// over --git-branch. +// +// TODO: the system should support specifying revision (refSpec) as a URL +// fragment throughout, which, when implemented, removes the need for the +// separate members. +func (c deployConfig) gitSource() fn.Git { + g := fn.Git{URL: c.GitURL, Revision: c.GitBranch, ContextDir: c.GitDir} + if parts := strings.SplitN(c.GitURL, "#", 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 +521,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 +529,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) } @@ -664,9 +711,7 @@ func (c deployConfig) Configure(f fn.Function) (fn.Function, error) { // Configure basic members f.Domain = c.Domain f.Namespace = c.Namespace - f.Build.Git.URL = c.GitURL - f.Build.Git.ContextDir = c.GitDir - f.Build.Git.Revision = c.GitBranch // TODO: should match; perhaps "refSpec" + f.Build.Git = c.gitSource() f.Build.RemoteStorageClass = c.RemoteStorageClass f.Deploy.ServiceAccountName = c.ServiceAccountName f.Deploy.ImagePullSecret = c.ImagePullSecret @@ -691,15 +736,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.GitURL, "#", 2); len(parts) == 2 { - f.Build.Git.URL = parts[0] - f.Build.Git.Revision = parts[1] - } return f, nil } @@ -720,9 +756,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 } @@ -932,21 +968,6 @@ func printDeployMessages(out io.Writer, f fn.Function) { if !f.Local.Remote && (f.Build.Git.URL != "" || f.Build.Git.Revision != "" || f.Build.Git.ContextDir != "") { fmt.Fprintf(out, "Warning: git settings are only applicable when running with --remote. Local source code will be used.") } - - // Git Branch Mismatch - // ------------------- - // When doing a remote build with --git-branch, 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.Git.URL != "" && f.Build.Git.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.Git.Revision { - fmt.Fprintf(out, "Warning: Local git branch '%s' does not match --git-branch '%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.Git.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 906993f54e..9b7f0391f7 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -535,6 +535,156 @@ func testFunctionContext(cmdFn commandConstructor, t *testing.T) { // TestDeploy_GitArgsPersist ensures that the git flags, if provided, are // persisted to the Function for subsequent deployments. +// gitFunction is a function as committed in a git repository: initialized, +// with the given name, and with no Root. +func gitFunction(name string) fn.Function { + return fn.NewFunctionWith(fn.Function{Name: name, Runtime: "go", Created: time.Now()}) +} + +// fromGit stands in for the git loader for the duration of the test, +// returning f for any source. The source requested is recorded in the +// returned value. +func fromGit(t *testing.T, f fn.Function) *fn.Git { + t.Helper() + old := newFunctionFromGit + t.Cleanup(func() { newFunctionFromGit = old }) + requested := &fn.Git{} + newFunctionFromGit = func(_ context.Context, g fn.Git) (fn.Function, error) { + *requested = g + return f, nil + } + return requested +} + +// 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 --git-url={url} +// +// https://github.com/knative/func/issues/3203 +func TestDeploy_RemoteGitNoLocalFunction(t *testing.T) { + root := FromTempDirectory(t) + requested := fromGit(t, gitFunction("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", + "--git-url=https://example.com/alice/remote-fn.git#feature", + "--git-dir=functions/remote-fn", + "--namespace=fnns"}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + // The requested source is exactly what the flags described + want := fn.Git{URL: "https://example.com/alice/remote-fn.git", Revision: "feature", ContextDir: "functions/remote-fn"} + if *requested != want { + t.Errorf("expected git source %+v, got %+v", want, *requested) + } + // The pipeline received the function from the repository, configured + // by the flags + 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.Git != 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) + fromGit(t, gitFunction("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", "--git-url=https://example.com/alice/remote-fn.git", "--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.Git.URL != "https://example.com/alice/remote-fn.git" { + t.Errorf("expected the git source to be recorded locally, got %q", local.Build.Git.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) + old := newFunctionFromGit + t.Cleanup(func() { newFunctionFromGit = old }) + newFunctionFromGit = func(context.Context, fn.Git) (fn.Function, error) { + return fn.Function{}, errors.New("revision \"nope\" not found") + } + + pipeliner := mock.NewPipelinesProvider() + cmd := NewDeployCmd(NewTestClient(fn.WithPipelinesProvider(pipeliner), fn.WithRegistry(TestRegistry))) + cmd.SetArgs([]string{"--remote", "--git-url=https://example.com/alice/remote-fn.git", "--git-branch=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") + } +} + func TestDeploy_GitArgsPersist(t *testing.T) { root := FromTempDirectory(t) @@ -549,6 +699,7 @@ func TestDeploy_GitArgsPersist(t *testing.T) { if err != nil { t.Fatal(err) } + fromGit(t, gitFunction("repo-fn")) // Deploy the Function specifying all of the git-related flags cmd := NewDeployCmd(NewTestClient( @@ -577,7 +728,8 @@ func TestDeploy_GitArgsPersist(t *testing.T) { } // TestDeploy_GitArgsUsed ensures that any git 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_GitArgsUsed(t *testing.T) { root := FromTempDirectory(t) @@ -591,6 +743,7 @@ func TestDeploy_GitArgsUsed(t *testing.T) { if err != nil { t.Fatal(err) } + requested := fromGit(t, gitFunction("repo-fn")) // A Pipelines Provider which will validate the expected values were received pipeliner := mock.NewPipelinesProvider() @@ -618,6 +771,9 @@ func TestDeploy_GitArgsUsed(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatal(err) } + if want := (fn.Git{URL: url, Revision: branch, ContextDir: dir}); *requested != want { + t.Errorf("expected the function to be read from %+v, got %+v", want, *requested) + } } // TestDeploy_GitURLBranch ensures that a --git-url which specifies the branch @@ -635,6 +791,7 @@ func TestDeploy_GitURLBranch(t *testing.T) { expectedUrl = "https://example.com/user/repo" expectedBranch = "branch" ) + requested := fromGit(t, gitFunction("repo-fn")) cmd := NewDeployCmd(NewTestClient( fn.WithDeployer(mock.NewDeployer()), fn.WithBuilder(mock.NewBuilder()), @@ -646,6 +803,9 @@ func TestDeploy_GitURLBranch(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatal(err) } + if requested.URL != expectedUrl || requested.Revision != expectedBranch { + t.Errorf("expected the function to be read from %q at %q, got %+v", expectedUrl, expectedBranch, *requested) + } f, err = fn.NewFunction(root) if err != nil { @@ -1619,6 +1779,7 @@ func TestDeploy_RemoteBuildURLPermutations(t *testing.T) { newTestFn := func(remote, build, url string) func(t *testing.T) { return func(t *testing.T) { root := FromTempDirectory(t) + fromGit(t, gitFunction("repo-fn")) // Create a new Function in the temp directory if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { @@ -1811,6 +1972,7 @@ func TestDeploy_UnsetFlag(t *testing.T) { } // Deploy it, specifying a Git URL + fromGit(t, gitFunction("f")) cmd := NewDeployCmd(NewTestClient()) cmd.SetArgs([]string{"--remote", "--git-url=https://git.example.com/alice/f"}) if err := cmd.Execute(); err != nil { @@ -3002,6 +3164,7 @@ func TestDeploy_RemoteExposeRecordsObservation(t *testing.T) { root := FromTempDirectory(t) cleanup := k8s.SetOpenShiftForTest(true, nil) defer cleanup() + fromGit(t, gitFunction("repo-fn")) if _, err := fn.New().Init(fn.Function{Runtime: "go", Root: root}); err != nil { t.Fatal(err) diff --git a/cmd/run.go b/cmd/run.go index f60ef3e9aa..5059d46795 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -154,11 +154,14 @@ func runRun(cmd *cobra.Command, newClient ClientFactory) (err error) { cfg runConfig f fn.Function ) - if cfg, err = newRunConfig(cmd).Prompt(); err != nil { + 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, err = fn.NewFunction(cfg.Path); err != nil { + if f, err = fn.NewFunction(cfg.Path); err != nil { // The prompt may change the path return } if !f.Initialized() { @@ -371,10 +374,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 6fdb998af7..e4b9b44297 100644 --- a/docs/reference/func_deploy.md +++ b/docs/reference/func_deploy.md @@ -57,6 +57,9 @@ DESCRIPTION eliminating the need for a local container engine. To trigger deployment of a git repository instead of local source, combine with '--git-url': 'func deploy --remote --git-url=git.example.com/alice/f.git' + The function is then read from the repository, so no local copy is + needed. Choose the revision with '--git-branch' and the directory within + the repository with '--git-dir'. Domain When deploying, a function's route is automatically generated using the diff --git a/pkg/functions/function.go b/pkg/functions/function.go index fd6d140eff..9f6192046b 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -418,12 +418,10 @@ func parseFunction(bb []byte) (f Function, err error) { } // Validate function is logically correct, returning a bundled, and quite -// verbose, formatted error detailing any issues. +// 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 { - if f.Root == "" { - return errors.New("function root path is required") - } - var ctr int errs := [][]string{ validateVolumes(f.Run.Volumes), @@ -546,6 +544,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) { From c611ec555aea61262a4306b5c6018edcfa4013e9 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 2 Sep 2026 21:00:33 +0200 Subject: [PATCH 5/6] test: drop the local checkout from the remote git e2e tests TestRemote_Source, TestRemote_Ref and TestRemote_Dir cloned the repository, checked out the branch or changed into the subdirectory before deploying, because the CLI read the function's metadata from the local func.yaml. The function now comes from the repository, so the tests deploy from an empty directory, which is what they set out to cover. --- e2e/e2e_remote_test.go | 55 ++++++------------------------------------ 1 file changed, 7 insertions(+), 48 deletions(-) diff --git a/e2e/e2e_remote_test.go b/e2e/e2e_remote_test.go index 653a32a765..600a110ade 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 --git-url={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", "--git-url", "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", "--git-url", "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 --git-dir={subdir} // func deploy --remote --git-dir={subdir} --git-url={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 --git-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", "--git-url", "https://github.com/functions-dev/func-e2e-tests", From 4ebfbf99d9df2e2b8c92a5d81d9d2ae6bcfdccb7 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 3 Sep 2026 00:22:55 +0200 Subject: [PATCH 6/6] 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/source_commit_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 +++++++++++++++++++ 5 files changed, 65 insertions(+) diff --git a/pkg/functions/function_git_test.go b/pkg/functions/function_git_test.go index d7a910426b..5dbe4f99e8 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" @@ -24,6 +25,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/source_commit_test.go b/pkg/pipelines/tekton/source_commit_test.go index 59e602be41..a4d7aad25b 100644 --- a/pkg/pipelines/tekton/source_commit_test.go +++ b/pkg/pipelines/tekton/source_commit_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" @@ -15,6 +16,11 @@ import ( // returns its path and the full hash of that commit. func newGitRepo(t *testing.T) (dir, commit string) { 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 +}