addrs: return error on ParsePluginSource

The ParsePluginSource function can be invoked from either a HCL2 context
(when parsing a required_plugins block), or from the command-line
itself.

While in the first context a hcl.Diagnostics is coherent, in case the
source to parse is a command-line argument, for example when installing
or removing a plugin, the error message cannot have an HCL context,
leading to errors that are incorrectly prefixed by a <nil> string dure
to the lack of a reference to attach the diagnostic to.

Therefore, in order to fix this behaviour, the logic that parses plugin
sources now returns an error, and attaching the error to an HCL subject
is done independently, if needed.

command/plugins_install.go

@@ -137,9 +137,9 @@ func (c *PluginsInstallCommand) RunContext(buildCtx context.Context, args *Plugi
 		opts.BinaryInstallationOptions.Ext = ".exe"
 	}
 
-	plugin, diags := addrs.ParsePluginSourceString(args.PluginIdentifier)
-	if diags.HasErrors() {
-		c.Ui.Error(diags.Error())
+	plugin, err := addrs.ParsePluginSourceString(args.PluginIdentifier)
+	if err != nil {
+		c.Ui.Errorf("Invalid source string %q: %s", args.PluginIdentifier, err)
 		return 1
 	}
 

command/plugins_remove.go

@@ -142,9 +142,9 @@ func (c *PluginsRemoveCommand) RunContext(buildCtx context.Context, args []strin
 		opts.BinaryInstallationOptions.Ext = ".exe"
 	}
 
-	plugin, diags := addrs.ParsePluginSourceString(args[0])
-	if diags.HasErrors() {
-		c.Ui.Error(diags.Error())
+	plugin, err := addrs.ParsePluginSourceString(args[0])
+	if err != nil {
+		c.Ui.Errorf("Invalid source string %q: %s", args[0], err)
 		return 1
 	}
 

hcl2template/addrs/plugin.go

@@ -9,7 +9,6 @@ import (
 	"path"
 	"strings"
 
-	"github.com/hashicorp/hcl/v2"
 	"golang.org/x/net/idna"
 )
 
@@ -112,9 +111,7 @@ func IsPluginPartNormalized(str string) (bool, error) {
 //
 //	namespace/name
 //	hostname/namespace/name
-func ParsePluginSourceString(str string) (*Plugin, hcl.Diagnostics) {
-	var diags hcl.Diagnostics
-
+func ParsePluginSourceString(str string) (*Plugin, error) {
 	var errs []string
 
 	if strings.HasPrefix(str, "/") {
@@ -152,23 +149,14 @@ func ParsePluginSourceString(str string) (*Plugin, hcl.Diagnostics) {
 			fmt.Fprintf(errsMsg, "* %s\n", err)
 		}
 
-		return nil, diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Malformed source URL",
-			Detail:   fmt.Sprintf("The provided source URL %q is invalid. The following errors have been discovered:\n%s\nA valid source looks like \"github.com/hashicorp/happycloud\"", str, errsMsg),
-		})
+		return nil, fmt.Errorf("The provided source URL is invalid.\nThe following errors have been discovered:\n%s\nA valid source looks like \"github.com/hashicorp/happycloud\"", errsMsg)
 	}
 
 	// check the 'name' portion, which is always the last part
 	_, givenName := path.Split(str)
 	_, err = ParsePluginPart(givenName)
 	if err != nil {
-		diags = diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Invalid plugin type",
-			Detail:   fmt.Sprintf(`Invalid plugin type %q in source %q: %s"`, givenName, str, err),
-		})
-		return nil, diags
+		return nil, fmt.Errorf(`Invalid plugin type %q in source: %s"`, givenName, err)
 	}
 
 	// Due to how plugin executables are named and plugin git repositories
@@ -194,44 +182,33 @@ func ParsePluginSourceString(str string) (*Plugin, hcl.Diagnostics) {
 			// the suggestedType to end up invalid here.)
 			suggestedType := strings.Replace(givenName, userErrorPrefix, "", -1)
 			if _, err := ParsePluginPart(suggestedType); err == nil {
-				return nil, diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid plugin type",
-					Detail: fmt.Sprintf("Plugin source %q has a type with the prefix %q, which isn't valid. "+
-						"Although that prefix is often used in the names of version control repositories for Packer plugins, "+
-						"plugin source strings should not include it.\n"+
-						"\nDid you mean %q?", str, userErrorPrefix, suggestedType),
-				})
+				return nil, fmt.Errorf("Plugin source has a type with the prefix %q, which isn't valid.\n"+
+					"Although that prefix is often used in the names of version control repositories "+
+					"for Packer plugins, plugin source strings should not include it.\n"+
+					"\nDid you mean %q?", userErrorPrefix, suggestedType)
 			}
 		}
 		// Otherwise, probably instead an incorrectly-named plugin, perhaps
 		// arising from a similar instinct to what causes there to be
 		// thousands of Python packages on PyPI with "python-"-prefixed
 		// names.
-		return nil, diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Invalid plugin type",
-			Detail: fmt.Sprintf("Plugin source %q has a type with the prefix %q, which isn't allowed "+
-				"because it would be redundant to name a Packer plugin with that prefix. "+
-				"If you are the author of this plugin, rename it to not include the prefix.",
-				str, redundantPrefix),
-		})
+		return nil, fmt.Errorf("Plugin source has a type with the %q prefix, which isn't valid.\n"+
+			"If you are the author of this plugin, rename it to not include the prefix.\n"+
+			"Ex: %q",
+			redundantPrefix,
+			strings.Replace(givenName, redundantPrefix, "", 1))
 	}
 
 	plug := &Plugin{
 		Source: str,
 	}
 	if len(plug.Parts()) > 16 {
-		return nil, diags.Append(&hcl.Diagnostic{
-			Severity: hcl.DiagError,
-			Summary:  "Too many parts to source URL",
-			Detail: fmt.Sprintf("The source URL must have at most 16 components, and the one provided has %d.\n"+
-				"This is unsupported by Packer, please consider using a source that has less components to it.\n"+
-				"If this is a blocking issue for you, please open an issue to ask for supporting more "+
-				"components to the source URI.",
-				len(plug.Parts())),
-		})
-	}
-
-	return plug, diags
+		return nil, fmt.Errorf("The source URL must have at most 16 components, and the one provided has %d.\n"+
+			"This is unsupported by Packer, please consider using a source that has less components to it.\n"+
+			"If this is a blocking issue for you, please open an issue to ask for supporting more "+
+			"components to the source URI.",
+			len(plug.Parts()))
+	}
+
+	return plug, nil
 }

hcl2template/addrs/plugin_test.go

@@ -33,15 +33,15 @@ func TestPluginParseSourceString(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, gotDiags := ParsePluginSourceString(tt.source)
+			got, err := ParsePluginSourceString(tt.source)
 			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("ParsePluginSourceString() got = %v, want %v", got, tt.want)
 			}
-			if tt.wantDiags && len(gotDiags) == 0 {
-				t.Errorf("Expected diags, but got none")
+			if tt.wantDiags && err == nil {
+				t.Errorf("Expected error, but got none")
 			}
-			if !tt.wantDiags && len(gotDiags) != 0 {
-				t.Errorf("Unexpected diags: %s", gotDiags)
+			if !tt.wantDiags && err != nil {
+				t.Errorf("Unexpected error: %s", err)
 			}
 		})
 	}

hcl2template/types.required_plugins.go

@@ -166,15 +166,15 @@ func decodeRequiredPluginsBlock(block *hcl.Block) (*RequiredPlugins, hcl.Diagnos
 			}
 
 			rp.Source = source.AsString()
-			p, sourceDiags := addrs.ParsePluginSourceString(rp.Source)
+			p, err := addrs.ParsePluginSourceString(rp.Source)
 
-			if sourceDiags.HasErrors() {
-				for _, diag := range sourceDiags {
-					if diag.Subject == nil {
-						diag.Subject = attr.Expr.Range().Ptr()
-					}
-				}
-				diags = append(diags, sourceDiags...)
+			if err != nil {
+				diags = diags.Append(&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Subject:  &rp.Requirement.DeclRange,
+					Summary:  "Failed to parse source",
+					Detail:   err.Error(),
+				})
 				continue
 			} else {
 				rp.Type = p

packer/plugin-getter/plugins_test.go

@@ -430,9 +430,9 @@ echo '{"version":"v2.10.0","api_version":"x6.1"}'`,
 
 			log.Printf("starting %s test", tt.name)
 
-			identifier, diags := addrs.ParsePluginSourceString("github.com/hashicorp/" + tt.fields.Identifier)
-			if len(diags) != 0 {
-				t.Fatalf("ParsePluginSourceString(%q): %v", tt.fields.Identifier, diags)
+			identifier, err := addrs.ParsePluginSourceString("github.com/hashicorp/" + tt.fields.Identifier)
+			if err != nil {
+				t.Fatalf("ParsePluginSourceString(%q): %v", tt.fields.Identifier, err)
 			}
 			cts, err := version.NewConstraint(tt.fields.VersionConstraints)
 			if err != nil {