[plugins] Use reflikes for plugins #1845

mikeland73 · 2024-02-26T19:54:54Z

Summary

This changes plugins to use new reflike struct. It brings it more in line with how flake refs work. For now, only some types are supported.

How was it tested?

CICD

This reverts commit b7eec16.

savil

Generally lgtm, but I was having trouble following some of the logic (see comments inline)

savil · 2024-02-28T18:17:26Z

nix/flake/flakeref.go

+	if allowSlashesIfRef {
+		split = strings.SplitN(upath, "/", 3)


I don't understand what scenario is being dealt with here. What does allowSlashesifRef do, and why does it only care about 3 slashes, while the else can deal with an unlimited number of slashes?

I think this is to fix a bug where we weren't parsing github refs with slashes in them correctly. I broke out the flakeref fixes into a separate PR just so they can be tracked separately - #1863.

savil · 2024-02-28T18:17:41Z

nix/flake/flakeref.go

@@ -206,7 +206,7 @@ func parseGitHubRef(refURL *url.URL, parsed *Ref) error {
 	// github:<owner>/<repo>(/<rev-or-ref>)?(\?<params>)?

 	parsed.Type = TypeGitHub
-	split, err := splitPathOrOpaque(refURL)
+	split, err := splitPathOrOpaque(refURL, true)


nit: comment bool param

savil · 2024-02-28T18:17:44Z

nix/flake/flakeref.go

@@ -129,7 +129,7 @@ func parseURLRef(ref string) (parsed Ref, fragment string, err error) {
 		// [flake:]<flake-id>(/<rev-or-ref>(/rev)?)?

 		parsed.Type = TypeIndirect
-		split, err := splitPathOrOpaque(refURL)
+		split, err := splitPathOrOpaque(refURL, false)


nit: comment bool param

savil · 2024-02-28T18:18:42Z

internal/plugin/reflike.go

+)
+
+// RefLike is like a flake ref, but in some ways more general. It can be used
+// to reference other types of files, e.g. devbox.json.


is the only difference that flake ref refer to directories, whereas here it can refer to a specific file? If so, could we call that out more strongly?

flake refs are specific to nix flakes and I wanted to avoid modifying that. The only real difference here is that there's an optional default filename the reflike refers to. In the case of plugins is devbox.json but I want to leave the door open to using reflikes for composing multiple projects as well if that is something we want to support in the future (In that case the filename is probably a devbox.json)

savil · 2024-02-28T18:20:49Z

internal/plugin/github.go

+		"https://raw.githubusercontent.com/",
+		p.Owner,
+		p.Repo,
+		lo.Ternary(p.Rev == "", "master", p.Rev),


to use golang std lib: cmp.Or(p.Rev, "master")

savil · 2024-02-28T18:22:42Z

internal/plugin/github.go

+		p.Owner,
+		p.Repo,
+		lo.Ternary(p.Rev == "", "master", p.Rev),
+		p.withFilename(p.Dir),


not sure I follow: why are we passing in p.Dir (a directory) as an argument for withFilename?

different plugin types have different fields represent the path. Local plugins use Path while github plugins use Dir

Maybe a cleaner solution could be to implement a configPath() function for each type. (though they would still both have some duplicate code)

savil · 2024-02-28T18:23:49Z

internal/plugin/github.go

-	repo     string
-	revision string
-	dir      string
+	RefLike


instead of embedding the struct, could we make it an explicit field to distinguish methods/fields that are from RefLike versus native to githubPlugin?

But its tolerable if it means having to re-implement a lot of methods unnecessarily

I can make that change. It does mean that plugin.Repo becomes plugin.ref.Repo so the code is a big uglier. (There's roughly 12 callsites in this file that would change)

savil · 2024-02-28T18:24:07Z

internal/plugin/github.go

-		p.dir,
+		p.Owner,
+		p.Repo,
+		lo.Ternary(p.Rev == "", "master", p.Rev),


consider: cmp.Or

gcurtis · 2024-02-28T19:32:42Z

Thanks for catching the flakeref parsing bugs. Still reviewing this PR, but I broke them out into a separate PR and added a couple tests - https://github.com/jetpack-io/devbox/pull/1863/files.

gcurtis

I'm having some trouble following the ref-like syntax. It sounds like it allows specifying specific files, but I'm not sure where it's parsing that (or maybe we're not yet?).

mikeland73 · 2024-02-28T20:59:29Z

I'm having some trouble following the ref-like syntax. It sounds like it allows specifying specific files, but I'm not sure where it's parsing that (or maybe we're not yet?).

That's actually a bug fixed in #1850. Will fix here as well.

mikeland73 · 2024-02-28T22:58:14Z

I'm having some trouble following the ref-like syntax. It sounds like it allows specifying specific files, but I'm not sure where it's parsing that (or maybe we're not yet?).

Actually this is fine here. RefLike takes a filename and uses withFilename to ensure the correct root config is used.

mikeland73 · 2024-02-29T01:03:03Z

Cleanup PR #1867

(The stacked nature makes it a bit tricky to make those changes here)

mikeland73 added 17 commits February 22, 2024 13:32

[plugins] Unify plugin config and devbox config

1ef5d29

Update readme -> description

6386016

Fix tests

8fa019e

Update lockfile

b98ab45

Bump timout

511bcbb

Bump timout to 10s

e67d8e6

Revert

1239bcc

Change port for lapp

b7eec16

Revert "Change port for lapp"

100a77f

This reverts commit b7eec16.

Merge branch 'main' into landau/plugins-unify

8866689

Merge branch 'main' into landau/plugins-unify

d8bb837

Merge branch 'main' into landau/plugins-unify

f8404d9

Fix

a7d0dbe

Merge branch 'main' into landau/plugins-unify

5e804f2

[plugins] Use reflikes for plugins

275fd49

Fix lint issue

dd716a7

fix builtins

8aaec6b

mikeland73 force-pushed the landau/shift-plugins-to-config branch from 2d41210 to 8aaec6b Compare February 26, 2024 21:59

Base automatically changed from landau/plugins-unify to main February 28, 2024 05:42

Merge branch 'main' into landau/shift-plugins-to-config

908f9a8

mikeland73 requested review from gcurtis and savil February 28, 2024 05:57

mikeland73 mentioned this pull request Feb 28, 2024

[plugins] Implement plugins v2 #1850

Merged

4 tasks

savil reviewed Feb 28, 2024

View reviewed changes

gcurtis reviewed Feb 28, 2024

View reviewed changes

Merge branch 'main' into landau/shift-plugins-to-config

8f7d329

Use cmp.Or

1bf645a

mikeland73 added 4 commits February 28, 2024 15:39

Requested changes, add test

3261194

More testability

cfa1b15

Merge branch 'main' into landau/shift-plugins-to-config

8a6f071

Fix test

c4c31c2

savil approved these changes Feb 29, 2024

View reviewed changes

mikeland73 merged commit 1cb6e09 into main Feb 29, 2024
24 checks passed

mikeland73 deleted the landau/shift-plugins-to-config branch February 29, 2024 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[plugins] Use reflikes for plugins #1845

[plugins] Use reflikes for plugins #1845

mikeland73 commented Feb 26, 2024 •

edited

Loading

savil left a comment

savil Feb 28, 2024

gcurtis Feb 28, 2024

savil Feb 28, 2024

savil Feb 28, 2024

savil Feb 28, 2024

mikeland73 Feb 28, 2024 •

edited

Loading

savil Feb 28, 2024

savil Feb 28, 2024

mikeland73 Feb 28, 2024

savil Feb 28, 2024

mikeland73 Feb 28, 2024

savil Feb 28, 2024

gcurtis commented Feb 28, 2024

gcurtis left a comment

mikeland73 commented Feb 28, 2024

mikeland73 commented Feb 28, 2024

mikeland73 commented Feb 29, 2024

[plugins] Use reflikes for plugins #1845

[plugins] Use reflikes for plugins #1845

Conversation

mikeland73 commented Feb 26, 2024 • edited Loading

Summary

How was it tested?

savil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeland73 Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcurtis commented Feb 28, 2024

gcurtis left a comment

Choose a reason for hiding this comment

mikeland73 commented Feb 28, 2024

mikeland73 commented Feb 28, 2024

mikeland73 commented Feb 29, 2024

mikeland73 commented Feb 26, 2024 •

edited

Loading

mikeland73 Feb 28, 2024 •

edited

Loading