[plugins] Fix github canonical name #1897
Conversation
| } | ||
|
|
||
| // Github only allows alphanumeric, hyphen, underscore, and period in repo names. | ||
| // but we clean up just in case. |
There was a problem hiding this comment.
why is the "but we clean up just in case" comment needed? The regex checks for the all the cases that Github allows for, which is good. Not understanding the "but..." as if we are doing some further restrictions?
There was a problem hiding this comment.
I just meant that we should never encounter weird stuff (because it would not be a valid github repo) but we sanitize it anyway just in case. (for example we really don't want a slash in there)
| // in github plugins. If it's missing, we just use the directory as the name. | ||
| name, _ := getPluginNameFromContent(plugin) | ||
| if name == "" { | ||
| name = strings.ReplaceAll(ref.Dir, "/", "-") |
There was a problem hiding this comment.
what happens if the plugin's devbox.json config is in the root of the github repo? Can we default ref.Dir to "root-folder" or something?
There was a problem hiding this comment.
If there's no name and it's in the root, the qualified name becomes owner.repo
| name = strings.ReplaceAll(ref.Dir, "/", "-") | ||
| } | ||
| plugin.name = githubNameRegexp.ReplaceAllString( | ||
| strings.Join(lo.Compact([]string{ref.Owner, ref.Repo, name}), "."), |
There was a problem hiding this comment.
is the lo.Compact needed? When would one of the ref.Owner, ref.Repo or name be empty?
There was a problem hiding this comment.
name can be empty if the plugin has no name and the dir is root.
| usererr.New( | ||
| "plugin %s is missing a required field 'name'", plugin.LockfileKey()) | ||
| } | ||
| if !nameRegex.MatchString(name) { |
There was a problem hiding this comment.
nit: lets move nameRegex to above this function since its used only here.
internal/plugin/github.go
Outdated
| plugin := &githubPlugin{ref: ref} | ||
| // For backward compatibility, we don't strictly require name to be present | ||
| // in github plugins. If it's missing, we just use the directory as the name. | ||
| name, _ := getPluginNameFromContent(plugin) |
There was a problem hiding this comment.
lets do a followup that checks the error to be ErrNameOmitted since right now it conflates malformed name with missing name.
Not blocking because I think the change could be a bit involved since you'd need to propagate the error from this function...
There was a problem hiding this comment.
actually, is distinguishing this scenario important for this code flow? I'm now having doubts....
|
Tested and confirmed that this works as expected |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Summary
Fixes #1848
github canonical names were
owner-repobut this breaks when there are multiple plugins in a single repo and creates possible conflicts with other plugins.This renames them to:
owner.repo.namewhere
nameis the name in the plugin.json. For backward compatibility we don't require the name to be present, if it's missing the name is the directory (with slashes replaced with hyphens)How was it tested?
Installed a few plugins from the same repo and inspected virtenv dir.