-
Notifications
You must be signed in to change notification settings - Fork 18
[PM-23193] Add ArchivedDate #422
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
Conversation
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 76.52% 76.94% +0.42%
==========================================
Files 269 270 +1
Lines 25312 25626 +314
==========================================
+ Hits 19371 19719 +348
+ Misses 5941 5907 -34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Vault changes look good 🚀.
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.
Looks fairly reasonable, some small suggestions
keywords.workspace = true | ||
|
||
[dependencies] | ||
reqwest = { workspace = true } |
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.
We should revert this change, we want to keep using workspace = true
for the dependencies to ensure they match with the ones we use on the other crates.
bitwarden-vault = { path = "crates/bitwarden-vault", version = "=1.0.0" } | ||
|
||
# External crates that are expected to maintain a consistent version across all crates | ||
async-trait = ">=0.1.80, <0.2" |
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 this split was intentional to separate the bitwarden owned crates from the external crates, and should be reverted as well.
crates/bitwarden-api-api/Cargo.toml
Outdated
serde = { version = "^1.0", features = ["derive"] } | ||
serde_json = "^1.0" | ||
serde_repr = "^0.1" | ||
serde_with = { version = "^3.8", default-features = false, features = ["base64", "std", "macros"] } |
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.
Same thing here as on the bitwarden-api-identity
FYI The server PR must be merged before this since we've updated the bindings. |
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.
question: What's going on with all the Cargo.toml
changes?
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.
@Hinton Ah The cargo changes were from the support/build-api
command. I didn't realize I needed to revert all of them. thanks for the heads up.
And yep, I'm going to wait for the server PR to be merged in first. thanks!
|
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.
@Jingo88 thank you for the discussion today!
Updated SDK to use an `archivedDate` in the ciphers.
🎟️ Tracking
Verify Archive Working in SDKPM-19156 - this is the more accurate ticket for tracking the work in this PR
📔 Objective
Updated SDK to use an
archivedDate
in the ciphers.Related Server PR
Related Clients PR
⏰ Reminders before review
team
🦮 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 confirmedissue 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