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

Conversation

avidal
Copy link
Contributor

@avidal avidal commented May 13, 2024

note: this is a ground-up rework of a previous PR for nested extensions: #6363

This patch introduces support for specifying load_host and git_subpath when registering an extension repository, each of which influences extension loading.

load_host

If an extension repo has a load_host specified, then extension discovery will attempt to locate an extension repository for an extension whose path starts with a matching load_host value unless the extension is otherwise already registered.

For example:

v1alpha1.extension_repo(name="custom", load_host="internal")

load("ext://internal/abc", "...")

In this case, the extension loading mechanism will first look for an already registered extension by that name, and if not found, will look for an extension repository with a load_host of internal, and then proceed to register the extension for that repo.

The roughly equivalent v1alpha1.extension declaration would be: v1alpha1.extension(name="internal_abc", repo="custom", ...)

git_subpath

In addition to a repo-wide auto registration host this patch introduces a git_subpath argument for non-file based extension repositories. This acts similarly to the repo_path argument for an extension, except it applies to all extensions located in that repository.

For example, combining load_host and git_subpath:

v1alpha1.extension_repo(name="custom", url="...", load_host="internal", git_subpath="extensions")
v1alpha1.extension(name="custom-root", repo_name="custom")
v1alpha1.extension(name="explicit", repo_name="custom", repo_path="explicit")

# Loads from custom:extensions/Tiltfile since the extension has no `repo_path`
load("ext://custom-root", "...")

# Loads from custom:extensions/explicit due to the extension being explicitly registered
load("ext://explicit", "...")

# Loads from custom:extensions/abc due to the registered `internal` load_host
load("ext://internal/abc", "...")

# Loads from custom:extensions/abc/def
load("ext://internal/abc/def", "...")

# Loads the tests/golang extension from the default extension repository
load("ext://tests/golang, "...")

Why?

At Datadog, we have a large Tilt extension library and most of our codebase is in a monorepo. The current extension loading mechanism makes it difficult to organize our extensions in such a way that's both maintainable by the extension authors and not onerous for the users.

These two changes provide a lot more flexibility for managing extension libraries. Using a repo wide subpath allows you to keep your extensions out of the repository root, and registering a load host allows your users to just drop a single extension_repo at the top of their Tiltfile.

With this, our complex extension meta loading extension can be reduced to a single v1alpha1.extension_repo line, and our users can load("ext://tiltlib/...")

@avidal
Copy link
Contributor Author

avidal commented May 13, 2024

For reviewers: note that each commit is logically separate:

  • A test for the existing "nesting" behavior against the default repository plus a side-effect test (a registered extension named one_two can be loaded via ext://one/two)
  • The repo prefix implementation
  • The repo path implementation
  • And a few more tests

@avidal
Copy link
Contributor Author

avidal commented May 13, 2024

Also: if repo.path is too far (or too complex) I think that just having a repo prefix would mostly solve our needs and I can back out that part.

@avidal avidal force-pushed the avidal/extension-autoregister branch from 084f5c1 to a651285 Compare May 13, 2024 20:28
@avidal avidal marked this pull request as ready for review May 13, 2024 21:04
internal/tiltfile/tiltextension/plugin.go Outdated Show resolved Hide resolved
pkg/apis/core/v1alpha1/extensionrepo_types.go Outdated Show resolved Hide resolved
pkg/apis/core/v1alpha1/extensionrepo_types.go Outdated Show resolved Hide resolved
@nicks
Copy link
Member

nicks commented May 14, 2024

(not sure what's going on with the compose v1 tests, lemme handle that in a separate pr)

@avidal
Copy link
Contributor Author

avidal commented May 15, 2024

(not sure what's going on with the compose v1 tests, lemme handle that in a separate pr)

Thanks, yeah. I looked at the CI logs and couldn't make heads or tails of it. I was just going to rerun and see if it fixed itself :)

@nicks
Copy link
Member

nicks commented May 15, 2024

ya, if you rebase, the Compose error should go away

@avidal avidal force-pushed the avidal/extension-autoregister branch from a651285 to f0ee888 Compare May 17, 2024 17:13
@avidal
Copy link
Contributor Author

avidal commented May 17, 2024

@nicks latest version is rebased and has a few more bits and bobs:

  • Changed prefix to load_host
  • Changed path to git_subpath
  • Return an error during repo registration for file:// repos that also have a git_subpath
  • Split up p.ensureExtension into several helper methods (register and register default) with their own tests

There's a few things I'd like to get your opinion on that I'll call out on specific file comments.

// 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.

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.

t.Skip()
}

t.Skip("cannot use file:// repos with git_subpath, making this test unusable")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the validation when registering an extension repository we cannot actually run this test after the change to block file:// repos with a git_subpath test.

I don't know if there's a simple way to support https:// repos in the test without requiring the network.

extName := apis.SanitizeName(moduleName)
extSet := objSet.GetOrCreateTypedSet(&v1alpha1.Extension{})

ext := p.registerDefaultExtension(nil /* *starlark.Thread */, extSet, extName, moduleName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these tests of the extension loading helpers are a bit crufty since I'm not sure how else to get a handle on a model, which I need for an objSet. I'm also not sure how to get a *starlark.Thread, but these functions don't actually use the thread so I pass in nil.

@avidal
Copy link
Contributor Author

avidal commented May 17, 2024

It appears the codegen check is failing in CI because of a field name mismatch (the triggering rule is names_match seemingly due to using LoadHost and load_host instead of LoadHost and loadHost.

Curiously, the ExtensionSpec uses RepoPath with a json and protobuf key of repoPath (to fit the name_match rule), but somehow the generated Starlark uses repo_path.

edit: Currently trying a rerun of the codegen after changing the json/protobuf field names to satisfy the rule. I have a hunch that the starlark codegen in-use translates camelCase to snake_case.

edit2: That does not seem to be the case. Changing field names to satisfy the rule results in the starlark API generating using camelCase.

edit3: It did work, I just misread the results of running codegen again.

@avidal avidal force-pushed the avidal/extension-autoregister branch from 92a2df7 to ff1d5f8 Compare May 17, 2024 18:03
Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

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

LGTM

this seems good! ya, there's some oddities in the test but they don't bother me too much, i can go back and do a pass on making them simpler

@nicks nicks merged commit 153ae0d into tilt-dev:master May 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants