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

tiltfile: use the extension api objects for file loading #5445

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

nicks
Copy link
Member

@nicks nicks commented Feb 3, 2022

Hello @milas, @landism,

Please review the following commits I made in branch nicks/extension:

119e28f (2022-02-03 16:59:23 -0500)
tiltfile: use the extension api objects for file loading
Now, extensions are stored in xdg home instead of being vendored in tilt_modules.

You can also change the default extension repo with:

v1alpha1.extension_repo(name='default', url='https://github.com/my-org/my-repo', ref='my-tag')

at the top of the tiltfile.

This should address the following issues:
#3188
#3787
#4707
#3426
#5336
#5171

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks nicks requested review from landism and milas February 3, 2022 22:02
@nicks
Copy link
Member Author

nicks commented Feb 3, 2022

most of this PR is:

  1. moving around deps
  2. adding public ForceApply() functions to the reconcilers
  3. swapping out the old impl with the new one

(1) and (2) could probably be a separate PR; they just make less sense in isolation.

Now, extensions are stored in xdg home instead of being vendored in tilt_modules.

You can also change the default extension repo with:

```
v1alpha1.extension_repo(name='default', url='https://github.com/my-org/my-repo', ref='my-tag')
```

at the top of the tiltfile.

This should address the following issues:
#3188
#3787
#4707
#3426
#5336
#5171
Copy link
Contributor

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

Added a few comments to consider

internal/tiltfile/tiltextension/plugin.go Outdated Show resolved Hide resolved
extStatus := e.extReconciler.ForceApply(ext, repoResolved)
if extStatus.Error != "" {
return "", fmt.Errorf("loading extension %s: %s", ext.Name, extStatus.Error)
} else if extStatus.Path == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for else here as well. it can collapse.

Comment on lines 146 to 149
if ext.ObjectMeta.Annotations == nil {
ext.ObjectMeta.Annotations = map[string]string{}
}
ext.ObjectMeta.Annotations[v1alpha1.AnnotationManagedBy] = "tiltfile.loader"
Copy link
Member

Choose a reason for hiding this comment

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

doesn't matter much, but TIL:

func SetMetaDataAnnotation(obj *ObjectMeta, ann string, value string) {

Comment on lines +139 to +143
v1alpha1.extension_repo(name='default', url='file://%s/my-custom-repo')
v1alpha1.extension(name='my-extension', repo_name='default', repo_path='my-custom-path')

load("ext://my-extension", "printFoo")
printFoo()
Copy link
Member

Choose a reason for hiding this comment

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

🎉

nicks added a commit to tilt-dev/tilt.build that referenced this pull request Feb 4, 2022
includes all the changes from tilt-dev/tilt#5445
on new extension download semantics
nicks added a commit to tilt-dev/tilt.build that referenced this pull request Feb 4, 2022
includes all the changes from tilt-dev/tilt#5445
on new extension download semantics
@landism
Copy link
Member

landism commented Feb 7, 2022

I got this output when tilt uping on a Tiltfile w/ uibuttons:

ERROR: extension uibutton: extension repo default not found
ERROR: extension uibutton: extension repo default not loaded yet

It looks like things worked anyway, but the output led me to believe it didn't.

@nicks
Copy link
Member Author

nicks commented Feb 7, 2022

@landism sent you a pr for the other issue!

@nicks nicks merged commit b509b25 into master Feb 7, 2022
@nicks nicks deleted the nicks/extension branch February 7, 2022 21:29
nicks added a commit to tilt-dev/tilt.build that referenced this pull request Feb 11, 2022
includes all the changes from tilt-dev/tilt#5445
on new extension download semantics
nicks added a commit to tilt-dev/tilt.build that referenced this pull request Feb 11, 2022
* docs: rewrite the extensions guide

includes all the changes from tilt-dev/tilt#5445
on new extension download semantics

* Update docs/extensions.md

Co-authored-by: Nick Sieger <nicksieger@gmail.com>

* Update docs/extensions.md

Co-authored-by: Nick Sieger <nicksieger@gmail.com>

Co-authored-by: Nick Sieger <nicksieger@gmail.com>
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.

3 participants