Surface module description in command outputs and errors when possible #3271

oliversun9 · 2024-08-27T17:48:12Z

This PR makes module.Description public to surface module description in command outputs and errors when possible.

This PR also adds a check for module description uniqueness in workspace provider, before passing them to the module set builder. This means we correctly return an user error when it's an user error, i.e. the buf.yaml v2 is not well formed.

error handling for cmds that expect uniq module paths; add todo test workspace test add comments update comments update comment rearrange cleanup update comment

author Oliver Sun <[email protected]> 1723165177 -0400 committer Oliver Sun <[email protected]> 1724277829 -0400 parent 06e3428 author Oliver Sun <[email protected]> 1723165177 -0400 committer Oliver Sun <[email protected]> 1724277651 -0400 add includes update tests build all modules underneath the input directory add newline fix lint update test add comments fix comment comment update comment fix write buf yaml file changelog only allow either includes or excludes update error message handle possible duplicate paths improve bucketID format update errors remove todo comment fix verb and add test update --exclude-path to allow pointing at a module clear module targets paths if module is excluded;add some test files update comment revert decision that allowed --exclude-path to point at module roots post merge new line update comment update update comment add comment comment rename update comment remove unused test files

…ssages (#3263)

…s to use it

github-actions · 2024-08-27T17:48:30Z

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`✅ passed`	`✅ passed`	`✅ passed`	Aug 27, 2024, 10:24 PM

oliversun9 · 2024-08-27T17:50:04Z

private/buf/bufctl/controller.go

@@ -1023,6 +1023,7 @@ func (c *controller) buildTargetImageWithConfigs(
 		c.logger.Debug(
 			"building image for target module",
 			zap.String("moduleOpaqueID", module.OpaqueID()),
+			zap.String("moduleDescription", module.Description()),


New:

buf lint foo --debug ... DEBUG building image for target module {"moduleOpaqueID": "foo-00001", "moduleDescription": "path: \"foo\", excludes: \"foo/bar\""} ...

We could also just get rid of the opaque ID here, but keeping it for debugging purpose.

oliversun9 · 2024-08-27T17:52:15Z

private/bufpkg/bufmodule/errors.go

-		_, _ = builder.WriteString(opaqueID)
-		if i != len(m.OpaqueIDs)-1 {
-			_, _ = builder.WriteString(` -> `)
+	_, _ = builder.WriteString("cycle detected in module dependencies:\n")


New:

buf dep graph Failure: cycle detected in module dependencies: path: "foo", excludes: "foo/bar" -> path: "foo", includes: "foo/bar/baz" -> path: "foo", includes: "foo/bar", excludes: "foo/bar/baz" -> path: "foo", excludes: "foo/bar"

oliversun9 · 2024-08-27T17:54:14Z

private/bufpkg/bufmodule/bufmoduleapi/uploader.go

@@ -449,7 +457,7 @@ func getV1Beta1ProtoUploadRequestContent(
 		return nil, syserror.New("expected local Module in getProtoLegacyFederationUploadRequestContent")
 	}
 	if module.ModuleFullName() == nil {
-		return nil, syserror.Newf("expected module name for local module %q", module.OpaqueID())
+		return nil, syserror.Newf("expected module name for local module %q", module.Description())


Description vs opaqueID does not really matter for syserrors, but I changed it just in case.

oliversun9 · 2024-08-27T17:54:22Z

private/bufpkg/bufmodule/bufmoduleapi/registry.go

@@ -125,7 +125,7 @@ func getSingleRegistryForContentModules(contentModules []bufmodule.Module) (stri
 	for _, module := range contentModules {
 		moduleFullName := module.ModuleFullName()
 		if moduleFullName == nil {
-			return "", syserror.Newf("expected module name for %q", module.OpaqueID())
+			return "", syserror.Newf("expected module name for %q", module.Description())


Description vs opaqueID does not really matter for syserrors, but I changed it just in case.

oliversun9 · 2024-08-27T18:00:00Z

private/buf/cmd/buf/command/dep/depgraph/depgraph.go

@@ -143,5 +143,5 @@ func moduleToString(module bufmodule.Module) string {
 		}
 		return moduleFullName.String()
 	}
-	return module.OpaqueID()
+	return module.Description()


New:

buf dep graph digraph { "buf.build/osun/separate" -> "path: "foo", excludes: "foo/bar"" "path: "foo", excludes: "foo/bar"" -> "path: "foo", includes: "foo/bar/baz"" "path: "foo", includes: "foo/bar/baz"" -> "path: "foo", includes: "foo/bar", excludes: "foo/bar/baz"" }

This output is escaped. Perhaps we should update the definition of Module.Description not to quote paths.

This is a breaking change and isn't desirable - definitely revert this

Reverted in 9f854e6

oliversun9 · 2024-08-27T18:02:51Z

private/buf/bufworkspace/workspace_provider.go

+			moduleConfig,
+		)
+		if _, ok := seenModuleDescriptions[moduleDescription]; ok {
+			return nil, fmt.Errorf("multiple module configs found with the same description: %q", moduleDescription)


New

buf build Failure: multiple module configs found with the same description: "path: \"foo\", excludes: \"foo/bar\""

The \" looks a bit awkward, should we update the definition of Module.Description not to quote paths?

Double checking, we want

Failure: ... the same description: "path: foo, excludes: foo/bar"
instead of
Failure: ... the same description: path: "foo", excludes: "foo/bar"
right?

the latter, which should come through if you use %s instead of %q

Updated in b4bc182

oliversun9 · 2024-08-27T18:04:07Z

private/bufpkg/bufmodule/bufmoduleapi/uploader.go

+					slicesext.Map(allDepModuleDescriptions, func(moduleDescription string) string { return "  " + moduleDescription }),
+					"\n",
+				),
+			)


Failure: all dependencies for module "buf.build/osun/separate" must be named but these modules are not: path: "foo", excludes: "foo/bar" path: "foo", includes: "foo/bar", excludes: "foo/bar/baz" path: "foo", includes: "foo/bar/baz"

oliversun9 · 2024-08-27T18:17:13Z

private/buf/bufworkspace/workspace_provider.go

+	//       - proot/foo
+	//   - path: proto
+	//     excludes:
+	//       - proot/foo


Should this check happen during bufYAMLFile parsing instead?

I think it's fine for this to happen in this layer, since WorkspaceProvider encapsulates the process of parsing the buf.yaml. I think this logic is pretty straightforward here.

…x with zeros

doriable · 2024-08-27T21:00:38Z

private/buf/bufworkspace/workspace_provider.go

+	//       - proot/foo
+	//   - path: proto
+	//     excludes:
+	//       - proot/foo


I think it's fine for this to happen in this layer, since WorkspaceProvider encapsulates the process of parsing the buf.yaml. I think this logic is pretty straightforward here.

oliversun9 and others added 30 commits August 21, 2024 17:56

bucketID := dirPath + (index == 0 ? '' : [index])

06e3428

error handling for cmds that expect uniq module paths; add todo test workspace test add comments update comments update comment rearrange cleanup update comment

post merge

947a195

fix changelog err

89df476

use normalpath.Join instead of filepath.Join when writing

07dbbc3

update changelog

f8a4f09

plural

3601af7

Merge branch 'main' into osun/dupath

49d51fa

update comments and some error messages

3423b11

address comment

e1d0e24

fix typo

5c4a885

add test

7b978a9

fix test

5a226f9

add test

d30244e

Includes() -> RootToIncludes()

becbcc8

additional comment

a99c9a3

typos

4d0a81c

Merge branch 'main' into osun/dupath

7d67db2

update comments

f0f153b

test each module is linted with the correct config

1924861

Merge branch 'main' into osun/dupath

9586b3a

Merge branch 'main' into osun/dupath

d12c032

remove mention of license in changelog

f57845b

reformat bucketID for v2 module configs

499a77b

add comment for looking up module configs by bucketIDs

b2f2a40

remove spurious slashes

9aaeb3b

remove another buf.yaml's spurious slashes

613b27b

fix bucketID formatting: // -> []

fc8d9f6

commit

4a426ab

Add mapping from module opaque ID to display name for better error me…

6a13722

…ssages (#3263)

oliversun9 added 4 commits August 27, 2024 10:47

Merge branch 'main' into osun/dupath

6f643db

make module.Description() public and update error messages and output…

a3a026b

…s to use it

validate unique module description before passing to ModuleSetBuilder

aa83453

typo

a3359a1

oliversun9 commented Aug 27, 2024

View reviewed changes

oliversun9 requested review from bufdev and doriable August 27, 2024 18:04

oliversun9 commented Aug 27, 2024

View reviewed changes

oliversun9 added 7 commits August 27, 2024 16:16

conditionally append index to the first occurence; no longer pad inde…

4c9118b

…x with zeros

misspelling

2cda8b0

bump changelog

6c5e02d

Merge branch 'main' into osun/dupath

826232b

Merge branch 'osun/dupath' into osun/make-description-public

343fbb0

still print OpaqueID for dep graph

9f854e6

%q -> %s for module description

b4bc182

doriable approved these changes Aug 27, 2024

View reviewed changes

move description

325444d

Base automatically changed from osun/dupath to main August 27, 2024 22:19

bufdev added 2 commits August 27, 2024 18:22

merge

cec09c1

fix

be52a4a

bufdev approved these changes Aug 27, 2024

View reviewed changes

bufdev merged commit 681874a into main Aug 27, 2024
11 checks passed

bufdev deleted the osun/make-description-public branch August 27, 2024 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Surface module description in command outputs and errors when possible #3271

Surface module description in command outputs and errors when possible #3271

oliversun9 commented Aug 27, 2024

github-actions bot commented Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024

oliversun9 Aug 27, 2024

oliversun9 Aug 27, 2024 •

edited

Loading

bufdev Aug 27, 2024

oliversun9 Aug 27, 2024

oliversun9 Aug 27, 2024 •

edited

Loading

bufdev Aug 27, 2024

oliversun9 Aug 27, 2024

bufdev Aug 27, 2024

oliversun9 Aug 27, 2024

oliversun9 Aug 27, 2024

oliversun9 Aug 27, 2024

doriable Aug 27, 2024

doriable Aug 27, 2024

Surface module description in command outputs and errors when possible #3271

Surface module description in command outputs and errors when possible #3271

Conversation

oliversun9 commented Aug 27, 2024

github-actions bot commented Aug 27, 2024 • edited Loading

oliversun9 Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

oliversun9 Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliversun9 Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliversun9 Aug 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024 •

edited

Loading

oliversun9 Aug 27, 2024 •

edited

Loading