Skip to content
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

Vera - api drive key unity & cli prep #17

Closed
wants to merge 77 commits into from

Conversation

organizedgrime
Copy link
Contributor

@organizedgrime organizedgrime commented Apr 29, 2024

various modifications in preparation for the api-drive key unification merge, along w some prep for CLI stuff.
https://linear.app/banyan/issue/ENG-589/api-and-drive-key-unification

Copy link

linear bot commented Apr 29, 2024

@organizedgrime organizedgrime marked this pull request as ready for review April 29, 2024 17:33
@organizedgrime organizedgrime changed the title Vera/eng 617 banyan cli Vera/eng 617 api drive key unity & cli prep Apr 29, 2024
@organizedgrime organizedgrime changed the title Vera/eng 617 api drive key unity & cli prep Vera - api drive key unity & cli prep Apr 29, 2024
Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This is partially a question for @sstelfox , how are we handling updating dependencies in our Cargo.lock it looks like the main branch had cargo update run over it since this branch diverged from it, which is essentially downgrading some of them. Do we have a process in place for when and how we update dependencies?

Copy link
Contributor

@sstelfox sstelfox Apr 30, 2024

Choose a reason for hiding this comment

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

We want to be able to consistently run cargo update on our dependencies and we shouldn't go backwards. Whenever there is a collision we should be able to just delete the Cargo.lock and run a fresh update.

If there is a broken version we're updating to we should pin that version and deal with fixing it in a separate PR (in practice I've never had to do this one the Rust community is pretty good about breaking changes).

We don't have an explicit update schedule. I'll perform an update usually whenever I'm adding/removign a new dependency or we get pinged about a known security vulnerability.

src/api/platform/account/create_user_key.rs Outdated Show resolved Hide resolved
src/api/platform/account/create_user_key.rs Outdated Show resolved Hide resolved
src/api/platform/account/user_key_access.rs Outdated Show resolved Hide resolved
src/api/platform/drive_access/revoke.rs Outdated Show resolved Hide resolved
src/api/platform/drives/mod.rs Outdated Show resolved Hide resolved
src/api/platform/models/api_drive_access.rs Outdated Show resolved Hide resolved
src/api/platform/models/api_user_key.rs Outdated Show resolved Hide resolved
src/wasm/tomb_compat/mod.rs Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ impl Nonce {
}

pub fn from_bytes(data: &[u8; NONCE_LENGTH]) -> Self {
Self(data.clone())
Self(*data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, super cool! I had no idea Copy is implemented for [T;N]

@jscatena88
Copy link
Contributor

I've looked through all the changes in filesystem and codec and they all look great to me!

Copy link
Contributor

@sstelfox sstelfox left a comment

Choose a reason for hiding this comment

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

Lots of changes! Wow. Most of it is spot on, there are couple of sections I've called out that should probably be tweaked a bit mostly out of concern for keeping our public SDK API clean and surprise free.

src/codec/meta/cid.rs Outdated Show resolved Hide resolved
src/filesystem/drive/directory_handle.rs Outdated Show resolved Hide resolved
src/stores/api_syncable_store/mod.rs Outdated Show resolved Hide resolved
@sstelfox sstelfox closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants