Workload & Mongodb Schema #51
Conversation
There was a problem hiding this comment.
I noticed that the delete fn is still unimplemented, so some of the feedback may be a bit premature. That said, I went ahead and finished the review since we discussed it via DM. Also, I do think that this is a great start! 💪
I really appreciate the clean up of the mongodb id type and the addition of the updated_at and deleted_at fields. I also responded to some of the questions re: the User collection and left some feedback on the update method; let me know if you have any Qs you'd like to sync on.
Co-authored-by: Lisa Jetton <[email protected]>
Co-authored-by: Lisa Jetton <[email protected]>
Co-authored-by: Lisa Jetton <[email protected]>
Co-authored-by: Lisa Jetton <[email protected]>
Co-authored-by: Lisa Jetton <[email protected]>
Co-authored-by: Lisa Jetton <[email protected]>
Co-authored-by: Lisa Jetton <[email protected]>
There was a problem hiding this comment.
Clean additions and changes! 💪 I'm liking the direction this is taking. I left some feedback on the mongodb schema and left a request to add some additional logic to the handle_db_update flow. I also proposed automating the metadata changes by moving them over the mongodb struct itself. (...and I love that new metadata structure, btw. :))
Let me know if you have any Qs. Happy to talk through any/all of this too.
| @@ -0,0 +1,17 @@ | |||
| use actix_web::{get, HttpResponse, Responder}; | |||
There was a problem hiding this comment.
Sync to async: we acknowledged that this is just a placeholder dir and will likely be replaced in the subsequent PR. That said, I am interested in seeing if we could use the actix code as the baseline for the orchestrator's external facing api.
| } | ||
|
|
||
| async fn db() -> mongodb::error::Result<()> { | ||
| let connection_uri = env::var("DB_CONNECTION_STRING").unwrap(); |
There was a problem hiding this comment.
Nudge... I know this code was just copied over from the holo-orchestrator repo and is subject to change, but while it lives in this repo and condition, I'd like to avoid adding any overlapping env vars. :)
What do you think about the suggestion above?
| pub enum Role { | ||
| Developer(DeveloperJWT), // jwt string | ||
| Host(HosterPubKey), // host pubkey | ||
| pub enum UserPermission { |
There was a problem hiding this comment.
I think this is a fine pattern, but I'm not sure of the use case. What are you envisioning the Permissions being used for?
There was a problem hiding this comment.
User's will generally require permissions. For example hosters should be able to access their own hosts and information about it. Developers should be able to access workloads they have deployed e.t.c.
This should be done through assigning permissions to them
There was a problem hiding this comment.
I see an overlap between this and the nats permissions use case. I understand that, prior to nats, we used to manage permissions in this way, but I think we no longer need to in our new architecture. That said, I'm open to being persuaded otherwise. :)
Signed-off-by: Zeeshan Abid <[email protected]>
Signed-off-by: Zeeshan Abid <[email protected]>
Signed-off-by: Zeeshan Abid <[email protected]>
Signed-off-by: Zeeshan Abid <[email protected]>
Signed-off-by: Zeeshan Abid <[email protected]>
Signed-off-by: Zeeshan Abid <[email protected]>
There was a problem hiding this comment.
I approve... with the small reminder/acknowledgement that the orchestrator code here is a placeholder and there are a few code debt items we expect to resolve in later prs. Also, let's make sure the CI passes prior to merging. 👍
Overall, I'm excited to see this move into main, this includes some updates to the mongodb interface and handling! Thanks for the hard work. 💪
|
@steveej - can you take a look at the above CI failure. It is failing with the error: |
this avoids the build-time download requirement.
i pushed d96abc1 to avoid the build-time download requirement entirely. |
|
Moved to #63 |
No description provided.