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

SM-417: Add project create, update, and delete commands #53

Merged
merged 8 commits into from
Jun 15, 2023

Conversation

coltonhurst
Copy link
Member

@coltonhurst coltonhurst commented May 24, 2023

Type of change

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

Objective

Adds the ability to create, update, and delete projects via the Bitwarden SDK.

This was originally under the SM-417 internal ticket, but is now more specifically represented by the child SM-783 internal ticket.

Code changes

  • client.rs: Add the project create, update, and delete commands for matching in the run function
  • client_projects.rs: Add the create, update, and delete functions
  • projects.rs: Add the implementations for the create_project, update_project, and delete_project project commands
  • command.rs: Add the project create, update, and delete commands to the ProjectsCommand enum
  • projects_request.rs: Add the ProjectCreateRequest, ProjectPutRequest, and the ProjectsDeleteRequest structs that are the request models
  • projects_response.rs: Add the ProjectsDeleteResponse and ProjectDeleteResponse structs that are the response models

All other schema update file changes were generated by npm run schemas.

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)

@coltonhurst coltonhurst self-assigned this May 24, 2023
@dani-garcia
Copy link
Member

Looks like a great start, thanks @coltonhurst!

A small thing missing, we have to add the new create, update, delete commands to the big command struct here:

pub enum ProjectsCommand {
/// > Requires Authentication
/// > Requires using an Access Token for login or calling Sync at least once
/// Retrieve a project by the provided identifier
///
/// Returns: [ProjectResponse](crate::sdk::response::projects_response::ProjectResponse)
///
Get(ProjectGetRequest),

And then use it here:

Command::Projects(cmd) => match cmd {

That way the new commands can be used both directly with the rust crate but also from the JSON wrapper and all the language bindings.

Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

Two minor comments, looks good otherwise!


#[derive(Serialize, Deserialize, Debug, JsonSchema)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
pub struct ProjectPutRequest {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be called updateRequest? Put feels like an implementation detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was to follow the convention in secrets_request.rs which uses SecretPutRequest. We definitely can refactor out Put in multiple locations and use Update though. This could be done in a different ticket, this one, or we take a different approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed this, we will address this in a separate PR

Comment on lines +54 to +57
let enc = client
.get_encryption_settings()
.as_ref()
.ok_or(Error::VaultLocked)?;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't enc already defined above? I think we can reuse it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, any recommendations on a better way to do this? By removing it we break one of the borrowing rules. With it removed we have the following error, the compiler delineates the following:

  • immutable borrow on line 35/36
  • mutable borrow on line 46
  • immutable borrow on line 54

cannot borrow *client as mutable because it is also borrowed as immutable

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed with @dani-garcia, addressing this in this PR: #70

Comment on lines +101 to +104
let enc = client
.get_encryption_settings()
.as_ref()
.ok_or(Error::VaultLocked)?;
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as here

Copy link
Member Author

Choose a reason for hiding this comment

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

Update here

@coltonhurst coltonhurst merged commit 4adfc04 into master Jun 15, 2023
@coltonhurst coltonhurst deleted the sm/SM-417 branch June 15, 2023 14:30
@Hinton Hinton mentioned this pull request Jul 17, 2023
1 task
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.

3 participants