-
Notifications
You must be signed in to change notification settings - Fork 174
feat(pack): add base digest pinning for adapters (best-effort) #1126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
78742a0
ae8d397
dd8e0a1
2e6f746
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,21 +18,26 @@ package pack | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "strings" | ||
|
|
||
| "github.com/kitops-ml/kitops/pkg/artifact" | ||
| cmdoptions "github.com/kitops-ml/kitops/pkg/cmd/options" | ||
| "github.com/kitops-ml/kitops/pkg/lib/constants" | ||
| "github.com/kitops-ml/kitops/pkg/lib/constants/mediatype" | ||
| "github.com/kitops-ml/kitops/pkg/lib/filesystem" | ||
| "github.com/kitops-ml/kitops/pkg/lib/filesystem/ignore" | ||
| kfutils "github.com/kitops-ml/kitops/pkg/lib/kitfile" | ||
| "github.com/kitops-ml/kitops/pkg/lib/repo/local" | ||
| "github.com/kitops-ml/kitops/pkg/lib/repo/remote" | ||
| "github.com/kitops-ml/kitops/pkg/lib/repo/util" | ||
| "github.com/kitops-ml/kitops/pkg/output" | ||
|
|
||
| ocispec "github.com/opencontainers/image-spec/specs-go/v1" | ||
| "oras.land/oras-go/v2/errdef" | ||
| ) | ||
|
|
||
| // runPack compresses and stores a modelkit based on a Kitfile. Returns an error if packing | ||
|
|
@@ -86,6 +91,17 @@ func pack(ctx context.Context, opts *packOptions, kitfile *artifact.KitFile, loc | |
| return nil, fmt.Errorf("Failed to resolve referenced modelkit %s: %w", kitfile.Model.Path, err) | ||
| } | ||
| extraLayerPaths = util.LayerPathsFromKitfile(parentKitfile) | ||
|
|
||
| baseDigest, err := resolveBaseDigest(ctx, opts.configHome, kitfile.Model.Path) | ||
| if err != nil { | ||
| output.Logf(output.LogLevelWarn, "failed to resolve base digest, keeping existing value: %v", err) | ||
| } else if baseDigest != "" { | ||
| if kitfile.Model.BaseDigest != "" && kitfile.Model.BaseDigest != baseDigest { | ||
| output.Logf(output.LogLevelWarn, "overriding user-specified baseDigest %s with resolved %s", | ||
| kitfile.Model.BaseDigest, baseDigest) | ||
| } | ||
| kitfile.Model.BaseDigest = baseDigest | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the point of being able to specify a base digest if it just gets overridden, pushing the digest mismatch into a single warning log line at build time. Imagine running this build in a CI -- will you notice that the digest has changed? If not, what was the point in including it? |
||
| } | ||
|
rishi-jat marked this conversation as resolved.
|
||
| } | ||
|
|
||
| ignore, err := ignore.NewFromContext(opts.contextDir, kitfile, extraLayerPaths...) | ||
|
|
@@ -112,6 +128,51 @@ func pack(ctx context.Context, opts *packOptions, kitfile *artifact.KitFile, loc | |
| return manifestDesc, nil | ||
| } | ||
|
|
||
| func resolveBaseDigest(ctx context.Context, configHome, baseRef string) (string, error) { | ||
| if strings.HasPrefix(baseRef, "sha256:") { | ||
| return baseRef, nil | ||
| } | ||
|
|
||
| ref, _, err := artifact.ParseReference(baseRef) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to parse base reference: %w", err) | ||
| } | ||
|
|
||
| localRepo, err := local.NewLocalRepo(constants.StoragePath(configHome), ref) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create local repository: %w", err) | ||
| } | ||
|
rishi-jat marked this conversation as resolved.
|
||
|
|
||
| desc, _, _, err := util.ResolveManifestAndConfig(ctx, localRepo, ref.Reference) | ||
| if err == nil { | ||
| return desc.Digest.String(), nil | ||
| } | ||
| // Expected cases: no base digest resolved locally (skip pinning, no remote fallback) | ||
| if errors.Is(err, util.ErrNoKitfile) || errors.Is(err, util.ErrNotAModelKit) { | ||
| return "", nil | ||
| } | ||
| if errors.Is(err, errdef.ErrNotFound) || errors.Is(err, errdef.ErrMissingReference) { | ||
| // Reference not found locally, try remote | ||
| } else { | ||
| // Unexpected local error (IO issues, corruption, etc.) | ||
| return "", fmt.Errorf("failed to resolve base digest locally: %w", err) | ||
| } | ||
|
Comment on lines
+150
to
+159
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment here contradicts the code that follows. |
||
|
|
||
| repository, err := remote.NewRepository(ctx, ref.Registry, ref.Repository, cmdoptions.DefaultNetworkOptions(configHome)) | ||
|
rishi-jat marked this conversation as resolved.
|
||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create remote repository: %w", err) | ||
| } | ||
|
|
||
| desc, _, _, err = util.ResolveManifestAndConfig(ctx, repository, ref.Reference) | ||
| if err != nil { | ||
| if errors.Is(err, util.ErrNoKitfile) || errors.Is(err, util.ErrNotAModelKit) { | ||
| return "", nil | ||
| } | ||
| return "", fmt.Errorf("failed to resolve base digest remotely: %w", err) | ||
| } | ||
|
rishi-jat marked this conversation as resolved.
|
||
| return desc.Digest.String(), nil | ||
| } | ||
|
|
||
| func readKitfile(modelFile string) (*artifact.KitFile, error) { | ||
| // 1. Read the model file | ||
| kitfile := &artifact.KitFile{} | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.