Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ linters:
goconst:
min-len: 3
min-occurrences: 5
ignore-tests: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -i 'golangci-lint|golangci' --glob '!vendor/**' .

Repository: crossplane/cli

Length of output: 722


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- .golangci.yml (around line 123) ---'
sed -n '110,135p' .golangci.yml

echo
echo '--- flake.nix (golangci-lint references) ---'
sed -n '150,185p' flake.nix

echo
echo '--- nix/checks.nix (golangci-lint references) ---'
sed -n '35,75p' nix/checks.nix

echo
echo '--- nix/apps.nix (golangci-lint references) ---'
sed -n '35,80p' nix/apps.nix

Repository: crossplane/cli

Length of output: 4276


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- flake.lock (nixpkgs inputs) ---'
rg -n '"nixpkgs"|github:NixOS/nixpkgs|rev|locked' flake.lock

Repository: crossplane/cli

Length of output: 884


🌐 Web query:

nixpkgs 1766437c5509f444c1b15331e82b8b6a9b967000 golangci-lint package version

💡 Result:

At commit 1766437c5509f444c1b15331e82b8b6a9b967000, which corresponds to the release-25.11 branch of nixpkgs [1][2], the golangci-lint package was at version 2.6.2 [1]. This version of the package was configured to be built using buildGo125Module [1]. As with other versions of golangci-lint in nixpkgs, this package is explicitly tied to a specific Go version to ensure compatibility, as the tool has historically required code changes to support new Go releases [3][1][4].

Citations:


Use linters.exclusions.rules for golangci-lint v2
.golangci.yml:123 is pinned through pkgs.unstable.golangci-lint 2.6.2, so goconst.ignore-tests no longer matches this version. Can you move the test-file exclusion to linters.exclusions.rules so it keeps working?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.golangci.yml at line 123, Update the golangci-lint configuration for
version 2.6.2 by removing the obsolete goconst ignore-tests setting and
expressing the test-file exclusion under linters.exclusions.rules, scoped to
goconst and test files.

Source: MCP tools

gocritic:
enabled-tags:
- performance
Expand Down
82 changes: 65 additions & 17 deletions internal/project/functions/python.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ package functions
import (
"bytes"
"context"
"fmt"
"io"
"net/http"
"path"
"path/filepath"
"strings"

"github.com/google/go-containerregistry/pkg/name"
v1 "github.com/google/go-containerregistry/pkg/v1"
Expand All @@ -42,15 +44,16 @@ import (
)

const (
// pythonBuildImage is the image in which we build the function. Its python
// version must match the python version of pythonRuntimeImage.
// pythonVersion is the Python version we use. Both the build image and the
// runtime image must have this version of Python installed.
pythonVersion = "3.13"

// pythonBuildImage is the image in which we build the function.
pythonBuildImage = "docker.io/library/debian:13-slim"
// pythonRuntimeImage is the distroless base used at runtime.
pythonRuntimeImage = "gcr.io/distroless/python3-debian13:nonroot"
// pythonBuildScript is the shell pipeline that runs in the build
// container. Mirrors function-template-python's Dockerfile: install hatch
// in a throwaway venv, build a wheel, install the wheel into a fresh venv
// at /fn.
// container.
//
// TODO(adamwg): We should build an image with python3 and python3-venv
// pre-installed so we don't have to install them for every build.
Expand All @@ -61,8 +64,18 @@ apt-get install -y --no-install-recommends python3 python3-venv
python3 -m venv /build
/build/bin/pip install --quiet hatch
/build/bin/hatch build -t wheel /whl
python3 -m venv /fn
/fn/bin/pip install --quiet /whl/*.whl
for arch in $ARCHS ; do
python3 -m venv /fn_$arch
/fn_$arch/bin/pip install \
--platform=manylinux2014_$arch \
--platform=manylinux_2_28_$arch \
--platform=manylinux_2_34_$arch \
--platform=manylinux_2_39_$arch \
--platform=manylinux_1_2_$arch \
--only-binary=:all: \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this cause issues if a function author wants a dep that doesn't ship a 2014 wheel? they're getting a little long in the tooth.

and according to my 🤖 the failure mode is a hard error that's not super informative

ERROR: No matching distribution found

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of the Python ecosystem is limited, but this makes sense to me. Based on the manylinux README, I've added --platform flags for all the manylinux versions that support amd64 and arm64. The pip docs say that's the way to indicate your runtime target supports multiple versions, and it works properly in my manual testing.

--target=/fn_$arch/lib/python$PY_VERSION/site-packages \
/whl/*.whl
done
`
)

Expand Down Expand Up @@ -101,7 +114,7 @@ func (b *pythonBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image,
return nil, errors.Wrap(err, "python builds require a Docker-compatible container runtime")
}

venvTar, err := b.buildVenv(ctx, c)
venvTars, err := b.buildVenv(ctx, c)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -130,7 +143,7 @@ func (b *pythonBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image,
}

venvLayer, err := tarball.LayerFromOpener(func() (io.ReadCloser, error) {
return io.NopCloser(bytes.NewReader(venvTar)), nil
return io.NopCloser(bytes.NewReader(venvTars[arch])), nil
})
if err != nil {
return errors.Wrap(err, "failed to create venv layer")
Expand All @@ -141,7 +154,7 @@ func (b *pythonBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image,
return errors.Wrap(err, "failed to append venv layer")
}

img, err = configurePythonImage(img)
img, err = configurePythonImage(img, arch)
if err != nil {
return errors.Wrap(err, "failed to configure python image")
}
Expand All @@ -154,14 +167,13 @@ func (b *pythonBuilder) Build(ctx context.Context, c BuildContext) ([]v1.Image,
return images, eg.Wait()
}

// buildVenv runs the build container against the function source and returns a
// tar of /fn suitable for use as an image layer (entries are rooted at
// /fn/...).
// buildVenv runs the build container against the function source and returns
// tars of /fn_<arch> for each architecture, suitable for use as an image layer.
//
// The function source is staged at /<FunctionPath> and, if a python schemas
// tree exists, /<SchemasPath>/python/ — preserving the project's relative
// layout so that pip resolves the schemas path-dep from pyproject.toml.
func (b *pythonBuilder) buildVenv(ctx context.Context, c BuildContext) ([]byte, error) {
func (b *pythonBuilder) buildVenv(ctx context.Context, c BuildContext) (map[string][]byte, error) {
fnFS := c.FunctionFS()
// Exclude any venv the user might have created in the function directory
// for local development, since (a) we don't need it, and (b) it will
Expand Down Expand Up @@ -191,8 +203,20 @@ func (b *pythonBuilder) buildVenv(ctx context.Context, c BuildContext) ([]byte,
buildImage = rewritten
}

pyArchitectures := make([]string, len(c.Architectures))
for i, a := range c.Architectures {
pyArchitectures[i], err = pythonArchitecture(a)
if err != nil {
return nil, err
}
}

opts := []docker.StartContainerOption{
docker.StartWithCopyFiles(fnTar, "/"),
docker.StartWithEnv(
"ARCHS="+strings.Join(pyArchitectures, " "),
"PY_VERSION="+pythonVersion,
),
docker.StartWithCommand([]string{"sh", "-c", pythonBuildScript}),
docker.StartWithWorkingDirectory("/" + filepath.ToSlash(c.FunctionPath)),
}
Expand All @@ -212,20 +236,44 @@ func (b *pythonBuilder) buildVenv(ctx context.Context, c BuildContext) ([]byte,
return nil, errors.Wrap(err, "python build container failed")
}

return docker.TarFromContainer(ctx, cid, "/fn")
ret := make(map[string][]byte)
for _, arch := range c.Architectures {
pyArch, _ := pythonArchitecture(arch) // Ignore the error since we already did this once.
ret[arch], err = docker.TarFromContainer(ctx, cid, fmt.Sprintf("/fn_%s", pyArch))
if err != nil {
return nil, errors.Wrapf(err, "failed to retrieve built function for architecture %s", arch)
}
}

return ret, nil
}

func pythonArchitecture(a string) (string, error) {
switch a {
case "amd64":
return "x86_64", nil
case "arm64":
return "aarch64", nil
default:
return "", errors.Errorf("unable to determine python architecture for architecture %s", a)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Error message could mention supported architectures.

"unable to determine python architecture for architecture %s" tells the user what failed but not what's supported. Per path instructions, error messages should be meaningful to end users and suggest next steps where possible.

💚 Suggested improvement
-default:
-  return "", errors.Errorf("unable to determine python architecture for architecture %s", a)
+default:
+  return "", errors.Errorf("unsupported architecture %s for Python functions: only amd64 and arm64 are supported", a)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return "", errors.Errorf("unable to determine python architecture for architecture %s", a)
return "", errors.Errorf("unsupported architecture %s for Python functions: only amd64 and arm64 are supported", a)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/project/functions/python.go` at line 247, Update the architecture
error returned by the Python architecture-detection logic to include the
supported architecture values and, where appropriate, guidance to use one of
them. Preserve the existing architecture value in the message and modify only
the error construction in the surrounding function.

Source: Path instructions

}
}

// configurePythonImage sets the runtime configuration on the final image to
// match function-template-python: nonroot user, the function entrypoint, and
// the gRPC port.
func configurePythonImage(img v1.Image) (v1.Image, error) {
func configurePythonImage(img v1.Image, arch string) (v1.Image, error) {
cfgFile, err := img.ConfigFile()
if err != nil {
return nil, errors.Wrap(err, "failed to get config file")
}
cfg := cfgFile.Config

cfg.Entrypoint = []string{"/fn/bin/function"}
pyArch, err := pythonArchitecture(arch)
if err != nil {
return nil, err
}
cfg.Entrypoint = []string{fmt.Sprintf("/fn_%s/lib/python%s/site-packages/bin/function", pyArch, pythonVersion)}
cfg.Cmd = nil
cfg.WorkingDir = "/"
cfg.User = "nonroot:nonroot"
Expand Down
Loading