Skip to content

App Distribution: Add commands to manage groups #5978

Merged
kaibolay merged 8 commits intomasterfrom
kb/more_group_management
Jun 15, 2023
Merged

App Distribution: Add commands to manage groups #5978
kaibolay merged 8 commits intomasterfrom
kb/more_group_management

Conversation

@kaibolay
Copy link
Copy Markdown
Contributor

@kaibolay kaibolay commented Jun 14, 2023

Description

  • Added appdistribution:group:create and appdistribution:group:delete.
  • Added --group-alias option to appdistribution:testers:add and appdistribution:testers:remove.

Sample Commands

$ firebase appdistribution:group:create "My Awesome Group" my-awesome-group
$ firebase appdistribution:testers:add --group-alias=my-awesome-group tester@one.tld tester@two.tld
$ firebase appdistribution:testers:remove --group-alias=my-awesome-group tester@one.tld tester@two.tld
$ firebase appdistribution:group:delete my-awesome-group
@kaibolay kaibolay requested a review from rebehe June 14, 2023 17:17
rebehe
rebehe approved these changes Jun 14, 2023
View reviewed changes
src/commands/appdistribution-group-create.ts
import { AppDistributionClient } from "../appdistribution/client";
import { getProjectName } from "../appdistribution/options-parser-util";

export const command = new Command("appdistribution:group:create <displayName> [alias]")
Copy link
Copy Markdown
Contributor

@rebehe rebehe Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should alias be a named option for clarity?
Copy link
Copy Markdown
Contributor Author

@kaibolay kaibolay Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it as an option before, but then changed my mind and decided against it. Mainly because it's an argument in group:delete - and I didn't want to change that.
src/commands/appdistribution-testers-add.ts
utils.logBullet(`Adding ${emailsToAdd.length} testers to project`);
await appDistroClient.addTesters(projectName, emailsToAdd);
if (options.groupAlias) {
utils.logBullet(`Adding ${emailsToAdd.length} testers to group`);
Copy link
Copy Markdown
Contributor

@rebehe rebehe Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Consider changing the plurality of testers depending on the length of the emails, like

firebase-tools/src/commands/ext-configure.ts

Line 145 in f5671f1

marked(`The following param${plural ? "s are" : " is"} immutable and won't be changed:`)

Copy link
Copy Markdown
Contributor Author

@kaibolay kaibolay Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm too lazy - there are too many places to change
@kaibolay kaibolay requested a review from bkendall June 14, 2023 19:49
@kaibolay kaibolay changed the title App Distribution: Add command to manage groups Jun 14, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (d4d1952) 54.91% compared to head (1b4d7e5) 54.95%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5978      +/-   ##
==========================================
+ Coverage   54.91%   54.95%   +0.04%     
==========================================
  Files         342      342              
  Lines       23247    23264      +17     
  Branches     4753     4754       +1     
==========================================
+ Hits        12766    12785      +19     
+ Misses       9351     9350       -1     
+ Partials     1130     1129       -1
Impacted Files Coverage Δ
src/appdistribution/client.ts 93.58% <100.00%> (+1.78%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.
@kaibolay kaibolay force-pushed the kb/more_group_management branch from f955bb1 to a670362 Compare June 14, 2023 21:07
@bkendall bkendall requested review from joehan and removed request for bkendall June 15, 2023 14:28
@kaibolay kaibolay requested review from bkendall and joehan and removed request for bkendall June 15, 2023 19:33
@kaibolay kaibolay merged commit d7ed2a9 into master Jun 15, 2023
@kaibolay kaibolay deleted the kb/more_group_management branch June 15, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@kaibolay @codecov-commenter @joehan @rebehe