-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add buf config ls-modules #3081
Conversation
This PR * adds test for `buf config ls-modules` * update the command to error when both `./buf.yaml` and `./buf.work.yaml` exist --------- Co-authored-by: bufdev <bufdev-github@buf.build>
@doriable will assume you will review my initial code and handle taking on any other necessary changes, I can't approve my own PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me.
} | ||
}() | ||
|
||
require.NoError(t, osext.Chdir(filepath.Join(pwd, "testdata", "lsmodules", "workspacev1"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be handy if buf config ls-modules
accepted an optional arg <dir>
so you could use it without relying on chdir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think --config
does that, except it doesn't accept a buf.work.yaml
&f.Config, | ||
configFlagName, | ||
"", | ||
`The buf.yaml file or data to use for configuration.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation first looks for a buf.work.yaml
file then falls back to a buf.yaml
file. Do we want the --config
argument to accept either a buf.yaml
or a buf.work.yaml
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good point:
- by default, the command looks for
./buf.work.yaml
or./buf.yaml
- but the
--config
currently only acceptsbuf.yaml
It does feel a bit inconsistent. Do you have thoughts on this? @bufdev @doriable @emcfarlane
Update the module name parsing to use `buf ls-modules` now we have `buf` at v1.34.0 as a requirement. This also fixes an issue for root named modules, which missed a call to `parseModules` causing the module to fail. Added a test to cover the root name case. See bufbuild/buf#3081
Fixes #3035.