Skip to content

Commit 4b481f5

Browse files
zhumin8zhumin8
authored
fix(internal/librarian): rm output flag from add command (#3701)
Removes output flag from add command. The use case for specifying a non-default output is rare. When it is needed, user can edit the librarian.yaml file manually between running `librarian add` and `librarian generate`. Also included changes: - refactor `addLibraryToLibrarianConfig` to return the cfg pointer for explicit mutations - fix remove redundant yaml.Write() Fix #3658
1 parent adb8cf6 commit 4b481f5

2 files changed

Lines changed: 13 additions & 73 deletions

File tree

‎internal/librarian/add.go‎

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,6 @@ func addCommand() *cli.Command {
3939
Name: "add",
4040
Usage: "add a new client library to librarian.yaml",
4141
UsageText: "librarian add <library> [apis...] [flags]",
42-
Flags: []cli.Flag{
43-
&cli.StringFlag{
44-
Name: "output",
45-
Usage: "output directory (optional, will be derived if not provided)",
46-
},
47-
},
4842
Action: func(ctx context.Context, c *cli.Command) error {
4943
args := c.Args()
5044
name := args.First()
@@ -55,12 +49,12 @@ func addCommand() *cli.Command {
5549
if len(args.Slice()) > 1 {
5650
channels = args.Slice()[1:]
5751
}
58-
return runAdd(ctx, name, c.String("output"), channels...)
52+
return runAdd(ctx, name, channels...)
5953
},
6054
}
6155
}
6256

63-
func runAdd(ctx context.Context, name, output string, channel ...string) error {
57+
func runAdd(ctx context.Context, name string, channel ...string) error {
6458
cfg, err := yaml.Read[config.Config](librarianConfigPath)
6559
if err != nil {
6660
return fmt.Errorf("%w: %w", errConfigNotFound, err)
@@ -73,20 +67,17 @@ func runAdd(ctx context.Context, name, output string, channel ...string) error {
7367
return fmt.Errorf("%w: %s", errLibraryAlreadyExists, name)
7468
}
7569

76-
if err := addLibraryToLibrarianConfig(cfg, name, output, channel...); err != nil {
77-
return err
78-
}
70+
cfg = addLibraryToLibrarianConfig(cfg, name, channel...)
7971
if err := RunTidyOnConfig(ctx, cfg); err != nil {
8072
return err
8173
}
8274
return nil
8375
}
8476

85-
func addLibraryToLibrarianConfig(cfg *config.Config, name, output string, channel ...string) error {
77+
func addLibraryToLibrarianConfig(cfg *config.Config, name string, channel ...string) *config.Config {
8678
lib := &config.Library{
8779
Name: name,
8880
CopyrightYear: strconv.Itoa(time.Now().Year()),
89-
Output: output,
9081
}
9182

9283
for _, c := range channel {
@@ -98,5 +89,5 @@ func addLibraryToLibrarianConfig(cfg *config.Config, name, output string, channe
9889
sort.Slice(cfg.Libraries, func(i, j int) bool {
9990
return cfg.Libraries[i].Name < cfg.Libraries[j].Name
10091
})
101-
return yaml.Write(librarianConfigPath, cfg)
92+
return cfg
10293
}

‎internal/librarian/add_test.go‎

Lines changed: 8 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ func TestAddLibrary(t *testing.T) {
3232
for _, test := range []struct {
3333
name string
3434
libName string
35-
output string
3635
initialLibraries []*config.Library
3736
wantFinalLibraries []*config.Library
3837
wantGeneratedOutputDir string
@@ -41,14 +40,12 @@ func TestAddLibrary(t *testing.T) {
4140
{
4241
name: "create new library",
4342
libName: "google-cloud-secretmanager",
44-
output: "newlib-output",
4543
initialLibraries: []*config.Library{},
4644
wantGeneratedOutputDir: "newlib-output",
4745
wantFinalLibraries: []*config.Library{
4846
{
4947
Name: "google-cloud-secretmanager",
5048
CopyrightYear: copyrightYear,
51-
Output: "newlib-output",
5249
},
5350
},
5451
},
@@ -57,8 +54,7 @@ func TestAddLibrary(t *testing.T) {
5754
libName: "google-cloud-secretmanager",
5855
initialLibraries: []*config.Library{
5956
{
60-
Name: "google-cloud-secretmanager",
61-
Output: "existing-output",
57+
Name: "google-cloud-secretmanager",
6258
},
6359
},
6460
wantGeneratedOutputDir: "existing-output",
@@ -67,11 +63,9 @@ func TestAddLibrary(t *testing.T) {
6763
{
6864
name: "create new library and tidy existing",
6965
libName: "google-cloud-secretmanager",
70-
output: "newlib-output",
7166
initialLibraries: []*config.Library{
7267
{
73-
Name: "existinglib",
74-
Output: "output",
68+
Name: "existinglib",
7569
APIs: []*config.API{
7670
{Path: "google/cloud/secretmanager/v1"},
7771
},
@@ -88,7 +82,6 @@ func TestAddLibrary(t *testing.T) {
8882
{
8983
Name: "google-cloud-secretmanager",
9084
CopyrightYear: copyrightYear,
91-
Output: "newlib-output",
9285
},
9386
},
9487
},
@@ -116,7 +109,7 @@ func TestAddLibrary(t *testing.T) {
116109
if err := yaml.Write(librarianConfigPath, cfg); err != nil {
117110
t.Fatal(err)
118111
}
119-
err = runAdd(t.Context(), test.libName, test.output)
112+
err = runAdd(t.Context(), test.libName)
120113
if test.wantError != nil {
121114
if !errors.Is(err, test.wantError) {
122115
t.Errorf("expected error %v, got %v", test.wantError, err)
@@ -151,11 +144,10 @@ func TestAddCommand(t *testing.T) {
151144

152145
testName := "google-cloud-secret-manager"
153146
for _, test := range []struct {
154-
name string
155-
args []string
156-
wantErr error
157-
wantAPIs []*config.API
158-
wantOutput string
147+
name string
148+
args []string
149+
wantErr error
150+
wantAPIs []*config.API
159151
}{
160152
{
161153
name: "no args",
@@ -206,31 +198,6 @@ func TestAddCommand(t *testing.T) {
206198
},
207199
},
208200
},
209-
{
210-
name: "library with multiple APIs and output flag",
211-
args: []string{
212-
"librarian",
213-
"add",
214-
testName,
215-
"google/cloud/secretmanager/v1",
216-
"google/cloud/secretmanager/v1beta2",
217-
"google/cloud/secrets/v1beta1",
218-
"--output",
219-
"packages/google-cloud-secret-manager",
220-
},
221-
wantOutput: "packages/google-cloud-secret-manager",
222-
wantAPIs: []*config.API{
223-
{
224-
Path: "google/cloud/secretmanager/v1",
225-
},
226-
{
227-
Path: "google/cloud/secretmanager/v1beta2",
228-
},
229-
{
230-
Path: "google/cloud/secrets/v1beta1",
231-
},
232-
},
233-
},
234201
} {
235202
t.Run(test.name, func(t *testing.T) {
236203
tmpDir := t.TempDir()
@@ -269,9 +236,6 @@ func TestAddCommand(t *testing.T) {
269236
if err != nil {
270237
t.Fatal(err)
271238
}
272-
if test.wantOutput != "" && got.Output != test.wantOutput {
273-
t.Errorf("output = %q, want %q", got.Output, test.wantOutput)
274-
}
275239
if test.wantAPIs != nil {
276240
if diff := cmp.Diff(test.wantAPIs, got.APIs); diff != "" {
277241
t.Errorf("channels mismatch (-want +got):\n%s", diff)
@@ -285,19 +249,16 @@ func TestAddLibraryToLibrarianYaml(t *testing.T) {
285249
for _, test := range []struct {
286250
name string
287251
libraryName string
288-
output string
289252
channels []string
290253
want []*config.API
291254
}{
292255
{
293256
name: "library with no specification-source",
294257
libraryName: "newlib",
295-
output: "output/newlib",
296258
},
297259
{
298260
name: "library with single API",
299261
libraryName: "newlib",
300-
output: "output/newlib",
301262
channels: []string{"google/cloud/storage/v1"},
302263
want: []*config.API{
303264
{
@@ -308,7 +269,6 @@ func TestAddLibraryToLibrarianYaml(t *testing.T) {
308269
{
309270
name: "library with multiple APIs",
310271
libraryName: "google-cloud-secret-manager",
311-
output: "output/google-cloud-secret-manager",
312272
channels: []string{
313273
"google/cloud/secretmanager/v1",
314274
"google/cloud/secretmanager/v1beta2",
@@ -343,15 +303,7 @@ func TestAddLibraryToLibrarianYaml(t *testing.T) {
343303
if err := yaml.Write(librarianConfigPath, cfg); err != nil {
344304
t.Fatal(err)
345305
}
346-
if err := addLibraryToLibrarianConfig(cfg, test.libraryName, test.output, test.channels...); err != nil {
347-
t.Fatal(err)
348-
}
349-
350-
cfg, err := yaml.Read[config.Config](librarianConfigPath)
351-
if err != nil {
352-
t.Fatal(err)
353-
}
354-
306+
cfg = addLibraryToLibrarianConfig(cfg, test.libraryName, test.channels...)
355307
if len(cfg.Libraries) != 2 {
356308
t.Errorf("libraries count = %d, want 2", len(cfg.Libraries))
357309
}
@@ -360,9 +312,6 @@ func TestAddLibraryToLibrarianYaml(t *testing.T) {
360312
if err != nil {
361313
t.Fatal(err)
362314
}
363-
if found.Output != test.output {
364-
t.Errorf("output = %q, want %q", found.Output, test.output)
365-
}
366315
if found.Version != "" {
367316
t.Errorf("version = %q, want %q", found.Version, "")
368317
}

0 commit comments

Comments
 (0)