Skip to content

[plugins] Fix init hooks for custom plugins #1500

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

Merged
merged 2 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions examples/plugins/local/my-plugin/my-plugin.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,10 @@
"{{ .Virtenv }}/some-file": "some-file.txt",
"{{ .DevboxDir }}/some-file.txt": "some-file.txt",
"{{ .Virtenv }}/process-compose.yaml": "process-compose.yaml"
},
"shell": {
"init_hook": [
"export MY_INIT_HOOK_VAR=BAR"
]
}
}
7 changes: 7 additions & 0 deletions examples/plugins/local/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,10 @@ if [ -z "$MY_FOO_VAR" ]; then
else
echo "MY_FOO_VAR is set to '$MY_FOO_VAR'"
fi

if [ -z "$MY_INIT_HOOK_VAR" ]; then
echo "MY_INIT_HOOK_VAR environment variable is not set."
exit 1
else
echo "MY_INIT_HOOK_VAR is set to '$MY_INIT_HOOK_VAR'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: fix indent. Same for line 7

fi
18 changes: 16 additions & 2 deletions internal/plugin/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,24 @@ import (
"go.jetpack.io/devbox/internal/devpkg"
)

func InitHooks(pkgs []*devpkg.Package, projectDir string) ([]string, error) {
func (m *Manager) InitHooks(
pkgs []*devpkg.Package,
includes []string,
) ([]string, error) {
hooks := []string{}
allPkgs := []Includable{}
for _, pkg := range pkgs {
c, err := getConfigIfAny(pkg, projectDir)
allPkgs = append(allPkgs, pkg)
}
for _, include := range includes {
name, err := m.ParseInclude(include)
if err != nil {
return nil, err
}
allPkgs = append(allPkgs, name)
}
for _, pkg := range allPkgs {
c, err := getConfigIfAny(pkg, m.ProjectDir())
if err != nil {
return nil, err
}
Expand Down
5 changes: 4 additions & 1 deletion internal/shellgen/scripts.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ func WriteScriptsToFiles(devbox devboxer) error {

// Write all hooks to a file.
written := map[string]struct{}{} // set semantics; value is irrelevant
pluginHooks, err := plugin.InitHooks(devbox.InstallablePackages(), devbox.ProjectDir())
pluginHooks, err := devbox.PluginManager().InitHooks(
devbox.InstallablePackages(),
devbox.Config().Include,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should this field be plural: Includes?

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 think this ship has sailed, has been in the devbox.json for a while now.

FWIW, I think we chatted about this an decided to use as a verb instead of noun (similar to import instead of imports). I don't feel super strongly, but it seems like it would be difficult to change (but I guess we could support both)

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah, I do remember chatting about it being a verb...

)
if err != nil {
return errors.WithStack(err)
}
Expand Down