[AC-2579] Set up bit-cli folder #9092

eliykat · 2024-05-09T04:40:24Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We're now adding Bitwarden Licensed code to the CLI client, so we need a separate bit-cli folder and build.

This PR does 2 things:

creates the bitwarden_license/bit-cli folder, config files, and entrypoint, and adds bit commands to apps/cli/package.json. This largely follows the bit-web structure.
refactors the CLI app to give more flexibility in registering arbitrary Programs depending on the entry point

Setting up CI builds is out of scope and will be done separately.

Code changes

Tools team: the only changes to your files are updating imports and naming.

Architecture devs: I would like you to review the substantive changes here.

The bit-cli config files and the new package.json commands are fairly self-explanatory, although they still need to be checked over.

The changes to the CLI app structure require a bit more explanation. At the moment, our services (from libs/common) and the CLI program classes are tightly coupled in the Main class, which is responsible for instantiating services, registering programs, and starting execution of the app. This is too many responsibilities and is quite rigid.

The refactored code takes advantage of Commander's builder pattern to give us more flexibility. The new code works like this:

ServiceContainer is responsible for initializing all services and making them available to programs (usually passed into their constructors)
registerOssPrograms is responsible for initializing and registering all (OSS) programs
bw.ts stitches it together: instantiate the service container, pass it to registerOssPrograms, start the app.

bit-cli has an identical structure:

ServiceContainer subclasses the OSS class and allows for registration of additional bit-licensed services
registerBitPrograms registers bit-licensed programs
bw.ts stitches it together, note that it calls registerOssPrograms first so that bit-cli is always a superset of the OSS functionality.

Screenshots

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team
Ensure that all UI additions follow WCAG AA requirements

* services are managed by the ServiceContainer * the app is bootstrapped by Main

codecov · 2024-05-09T04:47:04Z

Codecov Report

Attention: Patch coverage is 64.83516% with 96 lines in your changes are missing coverage. Please review.

Project coverage is 28.12%. Comparing base (6ab7336) to head (c633801).

Files	Patch %	Lines
apps/cli/src/service-container.ts	84.13%	28 Missing and 5 partials ⚠️
apps/cli/src/program.ts	0.00%	20 Missing ⚠️
apps/cli/src/commands/serve.command.ts	0.00%	12 Missing ⚠️
apps/cli/src/register-oss-programs.ts	0.00%	10 Missing ⚠️
bitwarden_license/bit-cli/src/bw.ts	0.00%	10 Missing ⚠️
apps/cli/src/bw.ts	0.00%	7 Missing ⚠️
apps/cli/src/vault.program.ts	0.00%	2 Missing ⚠️
apps/cli/src/tools/send/send.program.ts	0.00%	1 Missing ⚠️
...arden_license/bit-cli/src/register-bit-programs.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9092      +/-   ##
==========================================
+ Coverage   27.77%   28.12%   +0.35%     
==========================================
  Files        2422     2427       +5     
  Lines       70328    70345      +17     
  Branches    13124    13124              
==========================================
+ Hits        19531    19788     +257     
+ Misses      49277    49030     -247     
- Partials     1520     1527       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-09T05:04:19Z

Checkmarx One – Scan Summary & Details – 6ef196bc-6a35-47eb-9419-667106d66440

No New Or Fixed Issues Found

…-cli-folder

eliykat · 2024-05-10T04:08:45Z

apps/cli/src/service-container.ts

This file is actually just bw.ts renamed, with the run() method and all program classes removed. There are no changes to the rest of the file, e.g. how services are bootstrapped, this is not new code.

eliykat · 2024-05-10T04:14:19Z

bitwarden_license/bit-cli/src/bw.ts

+import { registerBitPrograms } from "./register-bit-programs";
+import { ServiceContainer } from "./service-container";
+
+async function main() {


One change (compared to the old Main.run() method) is that we used to explicitly check for no arguments and show help text: https://github.com/bitwarden/clients/blob/main/apps/cli/src/bw.ts#L740-L742. However, Commander does this automatically since version 5 and we're now on version 11: SO reference, Github reference. I've removed this code block, which also has the nice effect of removing any real logic from main - now it's all just initialization.

eliykat · 2024-05-10T04:30:44Z

I've refactored this since the draft yesterday and updated the description above.

audrey-jensen

Tools changes LGTM.

Kudos 👍🏻 - Loving how simple the entry point is now.
Thoughts 💭 - I'm guessing license issues don't apply to us due to CLAs, but it's still a little curious that we directly derive from the CLI's service class.
Nitpick ⛏️ - Can any of this be unit tested?

MGibson1

This looks just about perfect! well done. I want to understand the plan on script updates, but depending on your answer I'll approve as is.

MGibson1 · 2024-05-13T20:45:59Z

apps/cli/package.json

Please update normal build scripts to build:oss (see web scripts as pattern). If this is intended as part of your automation follow up, that's fine, too.

Will do! I avoided changing any existing scripts because that will require updates to Github workflows, which I didn't want to deal with here. But I will include it in the follow-up.

…-cli-folder

eliykat · 2024-05-14T00:29:30Z

Additional changes:

resolved merge conflicts - I had to manually reapply some changes to ServiceContainer, these conflicting PRs are referenced in the commits above
added bit (cli) to the vscode workspaces for those that use this feature
made inited a private property (something I added but forgot to mention - now that ServiceContainer.init() is public we want to make sure it's not called more than once)
added an extremely minimal unit test around ServiceContainer, at least to make sure it instantiates properly - although given that we have to manually order and instantiate our services in its constructor, this is not actually a given. It would be nice to test init(), but several services interact directly with the filesystem via fs, which would require more time setting up mocks etc than I want to give it here. (Also, maybe that starts to become an integration testing? Not sure; we don't have a good testing strategy for CLI.)

…-cli-folder

eliykat added 6 commits May 9, 2024 11:35

Create bit-cli folder with configs

f1c64dc

Fix webpack.config paths

ec0292d

Refactor CLI app structure

693b453

* services are managed by the ServiceContainer * the app is bootstrapped by Main

Fix ProgramDefinition type

7eb5428

Adopt builder pattern

dc00fcb

Add back eslint rule comment

0996148

eliykat requested review from Hinton and MGibson1 May 9, 2024 04:40

github-actions bot added the needs-qa Marks a PR as requiring QA approval label May 9, 2024

eliykat added 2 commits May 10, 2024 12:00

Merge remote-tracking branch 'origin/main' into ac/ac-2579/set-up-bit…

9d70fab

…-cli-folder

Simplify entry points

7045afc

eliykat force-pushed the ac/ac-2579/set-up-bit-cli-folder branch from dd4b74f to 7045afc Compare May 10, 2024 04:07

eliykat commented May 10, 2024

View reviewed changes

eliykat marked this pull request as ready for review May 10, 2024 04:30

eliykat requested a review from a team as a code owner May 10, 2024 04:30

eliykat requested review from addisonbeck and removed request for addisonbeck May 10, 2024 04:32

eliykat removed the needs-qa Marks a PR as requiring QA approval label May 10, 2024

audrey-jensen self-requested a review May 13, 2024 14:56

audrey-jensen previously approved these changes May 13, 2024

View reviewed changes

MGibson1 reviewed May 13, 2024

View reviewed changes

eliykat added 3 commits May 14, 2024 09:29

Merge remote-tracking branch 'origin/main' into ac/ac-2579/set-up-bit…

953eb5a

…-cli-folder

Reapply changes from #9099

e427abb

Reapply changes from #8604

9c3b3b1

eliykat dismissed audrey-jensen’s stale review via 9c3b3b1 May 13, 2024 23:37

Add to workspace

df59950

eliykat added 2 commits May 14, 2024 09:51

Make inited private

2055d87

Add unit tests for ServiceContainer

ca1e6be

eliykat requested review from audrey-jensen and MGibson1 May 14, 2024 00:30

This comment was marked as resolved.

Sign in to view

MGibson1 previously approved these changes May 14, 2024

View reviewed changes

eliykat added 3 commits May 15, 2024 08:21

Merge remote-tracking branch 'origin/main' into ac/ac-2579/set-up-bit…

21c9363

…-cli-folder

Reapply changes from #9115

2d78f02

Add warning

2f53e75

eliykat dismissed stale reviews from MGibson1 and audrey-jensen via 2f53e75 May 14, 2024 22:23

eliykat requested review from MGibson1 and audrey-jensen May 14, 2024 22:24

eliykat enabled auto-merge (squash) May 14, 2024 22:26

Merge branch 'main' into ac/ac-2579/set-up-bit-cli-folder

c633801

eliykat removed the request for review from Hinton May 14, 2024 22:32

MGibson1 approved these changes May 14, 2024

View reviewed changes

audrey-jensen approved these changes May 15, 2024

View reviewed changes

eliykat merged commit b14bb92 into main May 15, 2024
61 of 62 checks passed

eliykat deleted the ac/ac-2579/set-up-bit-cli-folder branch May 15, 2024 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AC-2579] Set up bit-cli folder #9092

[AC-2579] Set up bit-cli folder #9092

eliykat commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

eliykat May 10, 2024 •

edited

eliykat May 10, 2024 •

edited

eliykat commented May 10, 2024

audrey-jensen left a comment •

edited

MGibson1 left a comment

MGibson1 May 13, 2024

eliykat May 13, 2024

eliykat commented May 14, 2024 •

edited

This comment was marked as resolved.

[AC-2579] Set up bit-cli folder #9092

[AC-2579] Set up bit-cli folder #9092

Conversation

eliykat commented May 9, 2024 • edited

Type of change

Objective

Code changes

Screenshots

Before you submit

codecov bot commented May 9, 2024 • edited

Codecov Report

github-actions bot commented May 9, 2024 • edited

No New Or Fixed Issues Found

eliykat May 10, 2024 • edited

Choose a reason for hiding this comment

eliykat May 10, 2024 • edited

Choose a reason for hiding this comment

eliykat commented May 10, 2024

audrey-jensen left a comment • edited

Choose a reason for hiding this comment

MGibson1 left a comment

Choose a reason for hiding this comment

MGibson1 May 13, 2024

Choose a reason for hiding this comment

eliykat May 13, 2024

Choose a reason for hiding this comment

eliykat commented May 14, 2024 • edited

This comment was marked as resolved.

eliykat commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024 •

edited

eliykat May 10, 2024 •

edited

eliykat May 10, 2024 •

edited

audrey-jensen left a comment •

edited

eliykat commented May 14, 2024 •

edited