feat(dal,sdf,app): Reimplement variant_defs endpoints #3474

stack72 · 2024-03-26T20:28:11Z

As we are moving away from the concept of SchemaVariantDefinitions, we want to move the metadata that was part of a SchemaVariantDefinition to be part of the SchemaVariant

This will allow us to start bringing back the authoring workflow without the need to have the concept of a SchemaVariantDefinition

This PR also re-implements the get_variant_def endpoint as well as renaming variant_def to be variant in the API routes

stack72 · 2024-03-26T20:42:19Z

bors try

si-bors-ng · 2024-03-26T20:51:50Z

try

Build succeeded:

buildkite/si-merge-queue

stack72 · 2024-03-26T22:45:12Z

bors try

si-bors-ng · 2024-03-26T22:54:14Z

try

Build succeeded:

buildkite/si-merge-queue

stack72 · 2024-03-27T00:21:03Z

bors try

si-bors-ng · 2024-03-27T00:30:38Z

try

Build succeeded:

buildkite/si-merge-queue

nickgerace · 2024-03-27T18:59:54Z

app/web/src/store/asset.store.ts

@@ -104,7 +103,7 @@ export type AssetCloneRequest = Visibility & { id: AssetId };
 const LOCAL_STORAGE_LAST_SELECTED_ASSET_ID_KEY = "si-open-asset-id";

 export const assetDisplayName = (asset: Asset | AssetListEntry) =>
-  (asset.menuName ?? "").length === 0 ? asset.name : asset.menuName;
+  (asset.displayName ?? "").length === 0 ? asset.name : asset.displayName;


Yes. menuName was very old and we should have refactored it long ago.

lib/dal/src/action.rs

lib/dal/src/func.rs

nickgerace · 2024-03-27T19:03:20Z

lib/dal/src/pkg/import.rs

+                set_default_schema_variant_id(
+                    ctx,
+                    change_set_id,
+                    &mut schema,
+                    schema_spec
+                        .data()
+                        .as_ref()
+                        .and_then(|data| data.default_schema_variant()),
+                    variant_spec.unique_id(),
+                    variant.id(),
+                )
+                .await?;


lib/dal/src/pkg/import.rs

nickgerace · 2024-03-27T19:05:03Z

lib/dal/src/schema/variant.rs

+    pub fn display_name(&self) -> Option<String> {
+        self.display_name.clone()
+    }
+
+    pub fn link(&self) -> Option<String> {
+        self.link.clone()
+    }
+
+    pub fn description(&self) -> Option<String> {
+        self.description.clone()
+    }


nit: feel free to ignore. We could use self.<option-field>.as_deref() so that we allow the caller to decide to clone.

nickgerace · 2024-03-27T19:05:59Z

lib/dal/src/schema/variant.rs

+
+#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct SchemaVariantMetadataJson {


I love it! Great doc comments.

nickgerace · 2024-03-27T19:06:13Z

lib/dal/src/schema/variant.rs

+}
+
+impl SchemaVariantJson {
+    //     pub fn to_spec(


Are we keeping this commented out because it is functionality we wish to restore soon?

this is functionality that will come back!

nickgerace · 2024-03-27T19:06:37Z

lib/dal/src/schema/variant.rs

+                .unwrap_or(DEFAULT_SCHEMA_VARIANT_COLOR.into()),
+            component_type: variant_spec_data.component_type.into(),
+            link: variant_spec_data.link.as_ref().map(|l| l.to_string()),
+            description: None, // XXX - does this exist?


Let's remove it in a follow up PR if not!

nickgerace · 2024-03-27T19:07:21Z

lib/sdf-server/src/server/service/func.rs

@@ -510,6 +510,70 @@ pub async fn get_func_view(ctx: &DalContext, func: &Func) -> FuncResult<GetFuncR
    })
 }

+pub fn compile_return_types(ty: FuncBackendResponseType, kind: FuncBackendKind) -> &'static str {


nickgerace · 2024-03-27T20:28:10Z

Three things:

I was on autopilot and accidentally brought my review style for Reimplement attributes view for component debug screen #3475. I was specifically asked for a nit-heavy, style and convention scoped review there. Apologies to @stack72!
I'm going through my original review here to delete, edit, and/or comment on existing comments.
@stack72 let me know that this isn't done yet too, so I'll keep that in mind when doing the above.

nickgerace

Followed up on #3474 (comment) and the comments have been cleaned up accordingly.

Great job!

As we are moving away from the concept of SchemaVariantDefinitions, we want to move the metadata that was part of a SchemaVariantDefinition to be part of the SchemaVariant This will allow us to start bringing back the authoring workflow without the need to have the concept of a SchemaVariantDefinition

By creating an edge between these nodes on the graph, we can understand what Asset func was used to create the schema variant

This still needs to be enhanced with all of the funcs associated with a schema variant. We need to ensure that those funcs are included as edges during the import process of a package

This was originally an edge but as this is a 1:1 mapping of func to schema variant it creates, we can make it part of the NodeWeight itself

…riant

Variant definitions no longer exist so we now use variants as the route instead

keeb · 2024-03-28T21:33:49Z

lgtm, executed basic sanity test of product and everything that should work is working

stack72 · 2024-03-28T21:53:44Z

bors try

si-bors-ng · 2024-03-28T22:03:37Z

try

Build succeeded:

buildkite/si-merge-queue

nickgerace

🚀🚀🚀🚀🚀🚀🚀🚀🚀

stack72 · 2024-03-28T22:21:36Z

bors merge

si-bors-ng · 2024-03-28T22:31:18Z

Build succeeded:

buildkite/si-merge-queue

3497: Remove unused schema variant def module and unused auth prototype code (ENG-2426) r=nickgerace a=nickgerace ## Description `SchemaVariantDefinition` no longer exists in the new engine, so this PR removes its remains from the dal. <img src="https://media0.giphy.com/media/kVgCBJOy3PuHdqT5xQ/giphy.gif"/> ## Additional Note This is a follow-up to #3474. Co-authored-by: Nick Gerace <nick@systeminit.com>

github-actions bot added A-sdf Area: Primary backend API service [Rust] A-dal A-web A-dal-test labels Mar 26, 2024

stack72 force-pushed the feat-dal-sdf-app-Merge-the-concepts-of-Schema-Variant-and-Definition branch from 7b17567 to 0ed72ce Compare March 26, 2024 20:36

si-bors-ng bot added a commit that referenced this pull request Mar 26, 2024

Try #3474:

751390c

stack72 requested review from nickgerace and britmyerss March 26, 2024 20:42

si-bors-ng bot added a commit that referenced this pull request Mar 26, 2024

Try #3474:

2fd9288

stack72 force-pushed the feat-dal-sdf-app-Merge-the-concepts-of-Schema-Variant-and-Definition branch from 8836957 to 67e467b Compare March 27, 2024 00:11

stack72 changed the title ~~feat(dal,sdf,app): Merge the concepts of Schema Variant and Definition~~ feat(dal,sdf,app): Reimplement variant_defs endpoints Mar 27, 2024

stack72 force-pushed the feat-dal-sdf-app-Merge-the-concepts-of-Schema-Variant-and-Definition branch 3 times, most recently from 580efa5 to d895c55 Compare March 27, 2024 00:19

si-bors-ng bot added a commit that referenced this pull request Mar 27, 2024

Try #3474:

b04edc7

nickgerace reviewed Mar 27, 2024

View reviewed changes

nickgerace approved these changes Mar 27, 2024

View reviewed changes

stack72 force-pushed the feat-dal-sdf-app-Merge-the-concepts-of-Schema-Variant-and-Definition branch 5 times, most recently from c2f3fdc to e6818ac Compare March 28, 2024 20:15

stack72 added 5 commits March 28, 2024 20:27

feat(dal): Adding an edge between the Authoring Func and Schema Variant

0ceccb5

By creating an edge between these nodes on the graph, we can understand what Asset func was used to create the schema variant

feat(sdf, dal): Reimplement get_variant_def endpoint

e0c21d0

This still needs to be enhanced with all of the funcs associated with a schema variant. We need to ensure that those funcs are included as edges during the import process of a package

feat(sdf, dal): Move Schema Variant Authoring func Id into NodeWeight

8b56830

This was originally an edge but as this is a 1:1 mapping of func to schema variant it creates, we can make it part of the NodeWeight itself

feat(dal, sdf): Getting all the functions associated with a schema va…

423189f

…riant

feat(sdf, web): Rename the variant_defs route

0e43468

Variant definitions no longer exist so we now use variants as the route instead

stack72 force-pushed the feat-dal-sdf-app-Merge-the-concepts-of-Schema-Variant-and-Definition branch from e6818ac to 0e43468 Compare March 28, 2024 20:28

stack72 added 2 commits March 28, 2024 21:10

chore(dal): Ensuring we test the SchemaVariant:all_funcs call

0111f7f

feat(sdf): Ensure that get_func uses the correct TS Types for the editor

350e3b3

si-bors-ng bot added a commit that referenced this pull request Mar 28, 2024

Try #3474:

f2550eb

nickgerace approved these changes Mar 28, 2024

View reviewed changes

si-bors-ng bot merged commit 637d939 into main Mar 28, 2024
9 checks passed

si-bors-ng bot deleted the feat-dal-sdf-app-Merge-the-concepts-of-Schema-Variant-and-Definition branch March 28, 2024 22:31

nickgerace mentioned this pull request Apr 2, 2024

Remove unused schema variant def module and unused auth prototype code (ENG-2426) #3497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dal,sdf,app): Reimplement variant_defs endpoints #3474

feat(dal,sdf,app): Reimplement variant_defs endpoints #3474

stack72 commented Mar 26, 2024 •

edited

Loading

stack72 commented Mar 26, 2024

si-bors-ng bot commented Mar 26, 2024

stack72 commented Mar 26, 2024

si-bors-ng bot commented Mar 26, 2024

stack72 commented Mar 27, 2024

si-bors-ng bot commented Mar 27, 2024

nickgerace Mar 27, 2024

nickgerace Mar 27, 2024

nickgerace Mar 27, 2024

nickgerace Mar 27, 2024

nickgerace Mar 27, 2024 •

edited

Loading

stack72 Mar 28, 2024

nickgerace Mar 27, 2024 •

edited

Loading

nickgerace Mar 27, 2024

nickgerace commented Mar 27, 2024

nickgerace left a comment

keeb commented Mar 28, 2024 •

edited

Loading

stack72 commented Mar 28, 2024

si-bors-ng bot commented Mar 28, 2024

nickgerace left a comment

stack72 commented Mar 28, 2024

si-bors-ng bot commented Mar 28, 2024

feat(dal,sdf,app): Reimplement variant_defs endpoints #3474

feat(dal,sdf,app): Reimplement variant_defs endpoints #3474

Conversation

stack72 commented Mar 26, 2024 • edited Loading

stack72 commented Mar 26, 2024

si-bors-ng bot commented Mar 26, 2024

try

stack72 commented Mar 26, 2024

si-bors-ng bot commented Mar 26, 2024

try

stack72 commented Mar 27, 2024

si-bors-ng bot commented Mar 27, 2024

try

nickgerace Mar 27, 2024

Choose a reason for hiding this comment

nickgerace Mar 27, 2024

Choose a reason for hiding this comment

nickgerace Mar 27, 2024

Choose a reason for hiding this comment

nickgerace Mar 27, 2024

Choose a reason for hiding this comment

nickgerace Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

stack72 Mar 28, 2024

Choose a reason for hiding this comment

nickgerace Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

nickgerace Mar 27, 2024

Choose a reason for hiding this comment

nickgerace commented Mar 27, 2024

nickgerace left a comment

Choose a reason for hiding this comment

keeb commented Mar 28, 2024 • edited Loading

stack72 commented Mar 28, 2024

si-bors-ng bot commented Mar 28, 2024

try

nickgerace left a comment

Choose a reason for hiding this comment

stack72 commented Mar 28, 2024

si-bors-ng bot commented Mar 28, 2024

stack72 commented Mar 26, 2024 •

edited

Loading

nickgerace Mar 27, 2024 •

edited

Loading

nickgerace Mar 27, 2024 •

edited

Loading

keeb commented Mar 28, 2024 •

edited

Loading