-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-12747] Move CollectionService and models to AC Team #11278
[PM-12747] Move CollectionService and models to AC Team #11278
Conversation
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11278 +/- ##
==========================================
- Coverage 33.28% 33.18% -0.10%
==========================================
Files 2741 2772 +31
Lines 85810 86061 +251
Branches 16367 16394 +27
==========================================
+ Hits 28559 28561 +2
- Misses 54987 55235 +248
- Partials 2264 2265 +1 ☔ View full report in Codecov by Sentry. |
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.
Could you please add @bitwarden/admin-console/common
as a dependency to the package.json
-files for libs/importer
, libs/tools/vault-export-core
and libs/tools/vault-export-ui
. This is currently not enforced nor used, but in preparation to move to something like https://nx.dev/ for building packages within our monorepo.
3ffaa7c
to
4610e65
Compare
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.
Auth changes look good!
@@ -19,6 +19,7 @@ | |||
}, | |||
"dependencies": { | |||
"@bitwarden/common": "file:../common", | |||
"@bitwarden/vault-export-core": "file:../tools/export/vault-export/vault-export-core" | |||
"@bitwarden/vault-export-core": "file:../tools/export/vault-export/vault-export-core", | |||
"@bitwarden/admin-console-common": "file:../admin-console/src/common" |
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.
@eliykat:
This package-name differs from the actually used package-name (@bitwarden/admin-console/common
). Same issue applies to the vault-export package.json files. The one used here is a valid package name. I hadn't caught that with your previous PR. While working on the exporters I ran into a similar issue with @bitwarden/vault-export/core
and @bitwarden/vault-export/ui
. These are invalid package names.
libs/admin-console
does not export the common
-folder as it's own package. It's only achieved by deeplinking within the tsconfigs. Should libs/admin-console/common
be a it's own package or should it just be exported via @bitwarden/admin-console
?
I'm still fine with approving this, to not block the ownership move, but would like to request a follow-up ticket to look into a naming and packaging scheme for libs/admin-console
.
The tools-team has chosen to separate their packages by features so we can easily build what we need and also only depend on things we need. We are obviously fortunate that our ownership splits up so nicely, which isn't as easy for the other teams. Collections for example are a very commonly used entity and overlap into a ton of domains.
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'll hit you up on Slack to discuss this further!
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 went ahead and fixed any issues with incorrect import-paths (admin-console/src/common)
…ove-collectionservice-to-ac-team
55af6ef
…ove-collectionservice-to-ac-team
d459031
🥳 |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12747
📔 Objective
Move
CollectionService
and associated models into@bitwarden/admin-console/common
.I also took the opportunity to rename the implementation to
DefaultCollectionService
per our current naming convention, and drop theAbstraction
alias where it was used.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes