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

Adds "create entity" and "create entity-alias" Vault api calls. #38

Closed

Conversation

is-atw
Copy link

@is-atw is-atw commented Jun 8, 2022

No description provided.

@is-atw is-atw changed the title Adds "create identity" and "create identity-alias" Vault api calls. Adds "create entity" and "create entity-alias" Vault api calls. Jun 8, 2022
@jmgilman jmgilman self-requested a review June 8, 2022 13:52
@jmgilman
Copy link
Owner

jmgilman commented Jun 8, 2022

Thanks for the PR!

A few things:

  1. Can you run cargo fmt? That will clear up the CI errors.
  2. Can you add a test?
  3. Do you need access to the content of the wrapper? I noticed you're using exec_with_no_result but still declaring the wrapper in your responses. This might be an additional request to allow access to the wrapper if that's what you're after.

@is-atw is-atw marked this pull request as draft June 14, 2022 10:40
@is-atw is-atw marked this pull request as ready for review June 14, 2022 11:22
@is-atw
Copy link
Author

is-atw commented Jun 29, 2022

@jmgilman Please review.

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

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

Thanks for your Pull request, and sorry for the delay.
It would be nice if you can add the complete identity secret engine, so we provide a feature complete implementation for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please name the folder identity instead of entity for the entity we can use separated modules like this auth

use super::responses::CreateEntityResponse;
use rustify_derive::Endpoint;
use std::fmt::Debug;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some Comments above your function for example.

Suggested change
/// ## Create Entity
/// This endpoint creates an entity.
///
/// * Path: /identity/entity
/// * Method: POST
/// * Response: [CreateEntityResponse]
/// * Reference: https://www.vaultproject.io/api-docs/dentity/entity#create-an-entity

Comment on lines +13 to +16
pub struct CreateEntityRequest {
pub name: String,
pub policies: String,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a full feature set of the API documentation for the endpoint.

Suggested change
pub struct CreateEntityRequest {
pub name: String,
pub policies: String,
}
pub struct CreateEntityRequest {
pub name: String,
pub id: Option<String>,
pub metadata: Option<HashMap<String, String>>,
pub policies: Option<Vec<String>>,
pub disabled: Option<bool>,
}

pub request_id: String,
pub lease_id: String,
pub renewable: bool,
pub lease_duration: i64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

lease_dorations are always positive seconds, so we should use u64

Suggested change
pub lease_duration: i64,
pub lease_duration: u64,

@stormshield-gt
Copy link
Collaborator

@is-atw do you have time and motivation to apply the suggested changes?
If not maybe we can switch to #64 , @slck seems to have implemented more or less the same features

@Haennetz
Copy link
Collaborator

Haennetz commented Feb 24, 2024

@is-atw do you have time and motivation to apply the suggested changes? If not maybe we can switch to #64 , @slck seems to have implemented more or less the same features

Thanks for mention the other pull request, I reviewed it as well and added a comment.

@Haennetz
Copy link
Collaborator

I close this issue becuase we merged #77

@Haennetz Haennetz closed this Mar 20, 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