Skip to content
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

feat(extensions): auto registration and repo subpaths #6371

Merged
merged 8 commits into from
May 20, 2024
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
6 changes: 5 additions & 1 deletion internal/controllers/core/extension/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,11 @@ func (r *Reconciler) apply(ext *v1alpha1.Extension, repo *v1alpha1.ExtensionRepo
return v1alpha1.ExtensionStatus{Error: fmt.Sprintf("extension repo not loaded: %s", ext.Spec.RepoName)}
}

absPath := filepath.Join(repo.Status.Path, ext.Spec.RepoPath, "Tiltfile")
// The absolute path to an extension's Tiltfile is a combination of:
// - path to the repository on disk (repo.Status.Path)
// - specified path for all extensions within the repository (repo.Spec.GitSubpath)
// - specified path for the extension (ext.Spec.RepoPath)
absPath := filepath.Join(repo.Status.Path, repo.Spec.GitSubpath, ext.Spec.RepoPath, "Tiltfile")

// Make sure the user isn't trying to use path tricks to "escape" the repo.
if !ospath.IsChild(repo.Status.Path, absPath) {
Expand Down
66 changes: 65 additions & 1 deletion internal/controllers/core/extension/reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,51 @@ func TestDefault(t *testing.T) {
})

assert.Equal(t, f.ma.Counts, []analytics.CountEvent{
analytics.CountEvent{
{
Name: "api.extension.load",
Tags: map[string]string{
"ext_path": "my-ext",
"repo_type": "file",
"repo_url_hash": tiltanalytics.HashSHA1(repo.Spec.URL),
},
N: 1,
},
})
}

// Assert that the repo extension path, if specified, is used for extension location
func TestRepoSubpath(t *testing.T) {
f := newFixture(t)
repo := f.setupRepoSubpath("subpath")

ext := v1alpha1.Extension{
ObjectMeta: metav1.ObjectMeta{
Name: "my-repo:my-ext",
},
Spec: v1alpha1.ExtensionSpec{
RepoName: "my-repo",
RepoPath: "my-ext",
Args: []string{"--namespaces=foo"},
},
}
f.Create(&ext)

f.MustGet(types.NamespacedName{Name: "my-repo:my-ext"}, &ext)

p := f.JoinPath("my-repo", "subpath", "my-ext", "Tiltfile")
require.Equal(t, p, ext.Status.Path)

var tf v1alpha1.Tiltfile
f.MustGet(types.NamespacedName{Name: "my-repo:my-ext"}, &tf)
require.Equal(t, tf.Spec, v1alpha1.TiltfileSpec{
Path: p,
Labels: map[string]string{"extension.my-repo_my-ext": "extension.my-repo_my-ext"},
RestartOn: &v1alpha1.RestartOnSpec{FileWatches: []string{"configs:my-repo:my-ext"}},
Args: []string{"--namespaces=foo"},
})

assert.Equal(t, f.ma.Counts, []analytics.CountEvent{
{
Name: "api.extension.load",
Tags: map[string]string{
"ext_path": "my-ext",
Expand Down Expand Up @@ -206,3 +250,23 @@ func (f *fixture) setupRepo() *v1alpha1.ExtensionRepo {
f.Create(&repo)
return &repo
}

func (f *fixture) setupRepoSubpath(subpath string) *v1alpha1.ExtensionRepo {
p := f.JoinPath("my-repo", subpath, "my-ext", "Tiltfile")
f.WriteFile(p, "print('hello-world')")

repo := v1alpha1.ExtensionRepo{
ObjectMeta: metav1.ObjectMeta{
Name: "my-repo",
},
Spec: v1alpha1.ExtensionRepoSpec{
URL: fmt.Sprintf("file://%s", f.JoinPath("my-repo")),
GitSubpath: subpath,
},
Status: v1alpha1.ExtensionRepoStatus{
Path: f.JoinPath("my-repo"),
},
}
f.Create(&repo)
return &repo
}
91 changes: 78 additions & 13 deletions internal/tiltfile/tiltextension/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ import (
"github.com/tilt-dev/tilt/pkg/logger"
)

const extensionPrefix = "ext://"
const defaultRepoName = "default"
const (
extensionPrefix = "ext://"
defaultRepoName = "default"
)

type Plugin struct {
repoReconciler ExtRepoReconciler
Expand Down Expand Up @@ -129,6 +131,75 @@ func (e *Plugin) LocalPath(t *starlark.Thread, arg string) (localPath string, er
// Otherwise, infers an extension object that points to the default repo.
func (e *Plugin) ensureExtension(t *starlark.Thread, objSet apiset.ObjectSet, moduleName string) *v1alpha1.Extension {
extName := apis.SanitizeName(moduleName)

extSet := objSet.GetOrCreateTypedSet(&v1alpha1.Extension{})
if existing, exists := extSet[extName]; exists {
ext := existing.(*v1alpha1.Extension)
metav1.SetMetaDataAnnotation(&ext.ObjectMeta, v1alpha1.AnnotationManagedBy, "tiltfile.loader")
return ext
}

repoSet := objSet.GetOrCreateTypedSet(&v1alpha1.ExtensionRepo{})

return e.registerExtension(t, extSet, repoSet, extName, moduleName)
}

// In cases where an extension is not already registered, this function will search for an extension
// repo that can satisfy the requested extension, with a fallback to an extension in the default
// repository.
func (e *Plugin) registerExtension(t *starlark.Thread, extSet, repoSet apiset.TypedObjectSet, extName, moduleName string) *v1alpha1.Extension {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vacillated quite a bit on what the sig should be for registerExtension and registerDefaultExtension. I wasn't sure if they should share the signature with ensureExtension and only take moduleName and objSet and reconstruct extName and the specific typed sets, or if they should take the already "prepared" variables from .ensureExtension.

Take a look @nicks, particularly at how these signatures manifest in the tests if you would.

loadHost, extPath, tryRegister := strings.Cut(moduleName, "/")

// Safety fallback (in case this is called without already previously calling ensureExtension)
existing := extSet[extName]
if existing != nil {
return existing.(*v1alpha1.Extension)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems a bit off. In a fuller implementation this would also set the tiltfile.loader annotation but then it's effectively incorporating ensureExtension. Maybe there's a better way to break up ensureExtension. I'll think on it.


// If the supplied module name does not contain a / then there's no point looking for matching
// extension repositories. We can just return an extension named extName in the default
// repository
if !tryRegister {
return e.registerDefaultExtension(t, extSet, extName, moduleName)
}

// Otherwise, look for a repository that can satisfy this lookup
for _, v := range repoSet {
repo := v.(*v1alpha1.ExtensionRepo)
if repo.Spec.LoadHost == "" || repo.Spec.LoadHost != loadHost {
continue
}

// This repo load_host matches the first component of the module name
// So we can register this as an extension on that repo
ext := &v1alpha1.Extension{
ObjectMeta: metav1.ObjectMeta{
Name: extName,
Annotations: map[string]string{
v1alpha1.AnnotationManagedBy: "tiltfile.loader",
},
},
Spec: v1alpha1.ExtensionSpec{
RepoName: repo.Name,
RepoPath: extPath,
},
}

extSet[extName] = ext
return ext
}

return e.registerDefaultExtension(t, extSet, extName, moduleName)
}

// Registers an extension named moduleName in the default extension repository. Used as a fallback.
func (e *Plugin) registerDefaultExtension(t *starlark.Thread, extSet apiset.TypedObjectSet, extName, moduleName string) *v1alpha1.Extension {
// Safety fallback
existing := extSet[extName]
if existing != nil {
return existing.(*v1alpha1.Extension)
}

defaultExt := &v1alpha1.Extension{
ObjectMeta: metav1.ObjectMeta{
Name: extName,
Expand All @@ -142,15 +213,7 @@ func (e *Plugin) ensureExtension(t *starlark.Thread, objSet apiset.ObjectSet, mo
},
}

typedSet := objSet.GetOrCreateTypedSet(defaultExt)
existing, exists := typedSet[extName]
if exists {
ext := existing.(*v1alpha1.Extension)
metav1.SetMetaDataAnnotation(&ext.ObjectMeta, v1alpha1.AnnotationManagedBy, "tiltfile.loader")
return ext
}

typedSet[extName] = defaultExt
extSet[extName] = defaultExt
return defaultExt
}

Expand Down Expand Up @@ -179,8 +242,10 @@ func (e *Plugin) ensureRepo(t *starlark.Thread, objSet apiset.ObjectSet, repoNam
return defaultRepo
}

var _ starkit.LoadInterceptor = (*Plugin)(nil)
var _ starkit.StatefulPlugin = (*Plugin)(nil)
var (
_ starkit.LoadInterceptor = (*Plugin)(nil)
_ starkit.StatefulPlugin = (*Plugin)(nil)
)

func MustState(model starkit.Model) State {
state, err := GetState(model)
Expand Down
Loading