Integrate orgs and projects by teisenbe · Pull Request #325 · oxidecomputer/omicron

teisenbe · 2021-10-20T18:46:45Z

Introduce the notion of projects belonging to organizations. This has broad impacts on our API routes, as mentioned in #308.

Resolves #308

This is the insertion half of a pattern for preventing inconsistency when one operation inserts a resource into a collection while another deletes the collection.

This allows implementing multiple variants on one collection type. For example, DatastoreCollection<User> on Organization and DatastoreCollection<Project> on Organization.

WIP

teisenbe · 2021-10-20T18:49:51Z

Hmm, I did something slightly wrong at least, since it grabbed the authn stuff. I think what happened was I merged main into the CTE branch but forgot to push, and then pushed this branch. I'll see if I can disentangle

teisenbe · 2021-10-20T18:50:51Z

Okay, looks good now

davepacheco · 2021-10-20T20:29:38Z

-            .await
-            .map_err(|e| {
+        let organization_id = project.organization_id;
+        Organization::insert_resource(


There's a lot of refactoring noise in this PR so it's easy to miss, but this is really slick. I love that this block is clear for readers, handles the concurrency issues, and produces a useful error for the various cases! Thanks again for doing this.

smklein · 2021-10-20T19:05:30Z

+     path = "/organizations/{organization_name}/projects/{project_name}",
 }]
-async fn projects_put_project(
+async fn organization_projects_put_project(


Does it make sense to prepend the "organization_" prefix to these endpoints? Kinda seems like we should keep the old naming scheme for these functions - technically everything is nested under "organization", so it gets kinda noisy.

I'd be fine dropping it. I was following the scheme that seemed to already be present on things like instances and disks

Yeah, the current scheme is documented in the file. I think it'd be worthwhile that we stick with some consistent, predictable naming scheme, especially since the names show up in the generated clients and changing the names requires clients to make code changes. That scheme doesn't have to match what we do today.

Right, this was documented as {collection_path}_{verb}_{object}.

It's not clear if "collection_path" is meant to be the whole path, or only the "outermost collection".

If it's the full path, we would need to add the organization_ prefix to all endpoints. As an example, we'd want project_disks_post to become organizations_projects_disk_post, but that seems... wordy.

If it's the "outermost collection: I think organization_put_project makes more sense than organization_projects_put_project - the inner "project" is implied.

Is there a strong opinion on this? I'd be inclined to leave it to a later refactor (since I think it also has implications for other endpoint names)

Yeah I'd make it a separate PR.

(Could do it together with organizations -> orgs)

…ects

teisenbe · 2021-10-26T16:57:17Z

I think I forgot to ping this last week, this should be ready for review

david-crespo · 2021-10-26T21:27:40Z

+    // child-resource generation number
+    let org_after_p1_creation =
+        datastore.organization_fetch(&make_name(org_name)).await.unwrap();
+    assert!(org_after_p1_creation.rcgen > org_after_creation.rcgen);


You could you do without create_datastore by using nexus here

let nexus = &cptestctx.server.apictx.nexus; let org_after_creation = nexus.organization_fetch(&make_name(org_name)).await.unwrap();

On one hand I can see why you'd want to talk to directly to the DB and avoid any indirection the Nexus wrapper might introduce. On the other hand I have a slight bias against doing asserts about internal implementation details from an integration test if possible, and if those were moved elsewhere it makes the question of talking to the DB moot.

Hmm, I'll move this to a test within the datastore.rs I think

david-crespo · 2021-10-26T21:38:50Z

+    type CollectionTimeDeletedColumn = organization::dsl::time_deleted;
+    type CollectionIdColumn = project::dsl::organization_id;
+}
+


this is so clean 👌 👌 👌 👌

david-crespo

Looks great, thanks for doing all this. I don't have a strong opinion about the datastore in integration test comment, just curious about your thoughts.

…ects

teisenbe added 22 commits October 13, 2021 18:10

Define CTE for RFD 192's collection insert pattern

54d017b

This is the insertion half of a pattern for preventing inconsistency when one operation inserts a resource into a collection while another deletes the collection.

WIP DO NOT SUBMIT AS IS: try to sort through some of the trait issues

d621414

Merge branch 'main' into child-resource-insert-cte

c277510

finish implementing

dfe4832

address simple comments

29ee693

make CollectionTable/ResourceTable implicit

c780b54

fix issues

8b2e50f

Parameterize DatastoreCollection on ResourceType

3115679

This allows implementing multiple variants on one collection type. For example, DatastoreCollection<User> on Organization and DatastoreCollection<Project> on Organization.

Merge branch 'main' into child-resource-insert-cte

ab380c2

Merge branch 'main' into child-resource-insert-cte

f761028

Force return type to ResourceType

698f5c2

rename ResType to ResourceType to reduce ambiguity

2caceaf

remove InsertStatementExt

05f9e8c

Address feedback

587c7f4

Merge branch 'main' into child-resource-insert-cte

2b04065

Integrate organizations and projects

de02ed3

WIP

plumb through organization_ids

7babad6

remove allow(dead_code)

e60ba72

Migrate tests

114324f

Fixup oxapi_demo

f5656f1

add new test

7b26981

adapt to changes to collection_insert

2a81b58

teisenbe requested a review from davepacheco October 20, 2021 18:46

teisenbe changed the base branch from child-resource-insert-cte to main October 20, 2021 18:50

teisenbe changed the base branch from main to child-resource-insert-cte October 20, 2021 18:50

teisenbe mentioned this pull request Oct 20, 2021

Define CTE for RFD 192's collection insert pattern #313

Merged

teisenbe added 2 commits October 20, 2021 12:01

Add doc example

26ebff3

complete doc comment

e8dba02

fix capitalization in comment

8d52d80

davepacheco reviewed Oct 20, 2021

View reviewed changes

Comment thread nexus/src/db/datastore.rs

smklein reviewed Oct 20, 2021

View reviewed changes

teisenbe added 4 commits October 20, 2021 15:04

Rewrite query

13d887d

compute the name of the primary key

1f60f96

Merge branch 'child-resource-insert-cte' into integrate-orgs-and-proj…

92c06ac

…ects

Add test for deleting a non-empty organization

dd6382a

teisenbe mentioned this pull request Oct 21, 2021

Nexus infrastructure for testing concurrent operation interleaving #330

Open

teisenbe added 6 commits October 21, 2021 11:19

Add test to check rcgen is bumped

5b8dacf

Merge branch 'main' into child-resource-insert-cte

7af7f7c

Merge branch 'child-resource-insert-cte' into integrate-orgs-and-proj…

a80dd92

…ects

Fix tests

2e8a88b

Merge branch 'main' into child-resource-insert-cte

501acba

Merge branch 'child-resource-insert-cte' into integrate-orgs-and-proj…

cde0d88

…ects

david-crespo reviewed Oct 26, 2021

View reviewed changes

david-crespo approved these changes Oct 26, 2021

View reviewed changes

Move rcgen tests to datastore.rs

6e78ad2

davepacheco approved these changes Oct 27, 2021

View reviewed changes

teisenbe added 3 commits October 27, 2021 09:24

Merge branch 'main' into child-resource-insert-cte

13e17aa

make update clause rely on found_row

cccd92d

Merge branch 'child-resource-insert-cte' into integrate-orgs-and-proj…

3c37736

…ects

Base automatically changed from child-resource-insert-cte to main October 27, 2021 17:16

Merge branch 'main' into integrate-orgs-and-projects

29d9879

teisenbe merged commit 4466bee into main Oct 27, 2021

teisenbe deleted the integrate-orgs-and-projects branch October 27, 2021 19:40

david-crespo mentioned this pull request Oct 30, 2021

Bump API (orgs change), hard code org oxidecomputer/console#494

Merged

Conversation

teisenbe commented Oct 20, 2021

Uh oh!

teisenbe commented Oct 20, 2021

Uh oh!

teisenbe commented Oct 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teisenbe commented Oct 26, 2021

Uh oh!

david-crespo Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

david-crespo Oct 26, 2021 •

edited

Loading