feat: expose flag to determine whether a composite is indexed #196

dbcfd · 2023-11-15T21:55:22Z

No description provided.

stbrody

basically looks good, adding Paul for naming convention questions

packages/cli/src/commands/composite/create.ts

PaulLeCam

Looks OK to me but it misses docs and tests.

packages/cli/src/commands/composite/create.ts

packages/cli/src/commands/composite/deploy.ts

stbrody

my main question is around the behavior of the --no-index to the composite deploy command. My understanding is that the very definition of "deploying" a composite means telling the node to index the constituent models.

packages/cli/src/commands/composite/deploy.ts

website/docs/api/commands/cli.composite.md

PaulLeCam

As pointed out by Spencer, I don't get what is the purpose for deploying without indexing?
That would mean documents can be created but not queried on the node, which seems unlikely to be the wanted user experience?

packages/cli/src/commands/composite/create.ts

packages/cli/src/commands/composite/deploy.ts

dbcfd · 2023-11-27T17:56:05Z

As pointed out by Spencer, I don't get what is the purpose for deploying without indexing? That would mean documents can be created but not queried on the node, which seems unlikely to be the wanted user experience?

The desired user behavior is to update their composite with additional indices for querying. That requires create without indexing. However, they may also run deploy, since this is the steps specified in docs, so if they do that, need to do so without indexing.

stbrody · 2023-11-30T22:59:00Z

packages/cli/src/commands/composite/create.ts

+      char: 'd',
+      description:
+        'Deploy the composite to the ceramic node, which will start indexing on the composite',
+      default: true,


I don't feel super strongly about it, but I lean towards making create not deploy by default, so users have to explicitly deploy when they're ready. I think this will help with more complex situations where devs wants to create and merge multiple composites, possibly adding indices and views along the way, before finally deploying the final unified composite as the last step.

I could be swayed on this though if there's a case for why it's better to deploy by default

Also, what does composite:from-model do? That one seems even more like it definitely shouldn't auto-deploy since you're likely to want to add indices to it before deploying. And if that one doesn't auto-deploy, then I feel like this shouldn't either just for consistency sake if nothing else

I was doing this just to maintain the existing behavior, so this change won't suddenly cause things to "stop working" for users that aren't calling deploy.

I'm not entirely sure which way is better either.

okay, I guess preserving existing behavior makes sense

packages/cli/test/composites.test.ts

packages/devtools-node/src/fs.ts

website/docs/api/commands/cli.composite.md

stbrody · 2023-11-30T23:09:03Z

website/docs/api/commands/cli.composite.md

@@ -117,6 +123,12 @@ available on the Ceramic Node that yor DApp connects to. You can find a detailed
 guide on Composites' deployment
 [here](https://developers.ceramic.network/docs/composedb/guides/data-modeling/composites#deploying-composites)

+If updating your composite to add additional query fields, do not run this command.


again, I think we need to be clearer with our language here about what exactly the problematic scenarios are and what the proper recourse is.

Did the language here get updated? I like the language you used for the create command above (mod one small suggestion), can we re-use that?

Adds a flag to deploy composite when creating, which is defaulted to false. Additionally adds a flag to disable or enable indexing when deploying a composite.

stbrody · 2023-12-05T21:29:02Z

packages/cli/src/commands/composite/create.ts

+      char: 'd',
+      description:
+        'Deploy the composite to the ceramic node, which will start indexing on the composite',
+      default: true,


okay, I guess preserving existing behavior makes sense

stbrody · 2023-12-05T21:45:04Z

packages/cli/src/commands/composite/from-model.ts

+      char: 'd',
+      description:
+        'Deploy the composite to the ceramic node, which will start indexing on the composite',
+      default: true,


If I'm reading the code correctly, I believe this is a behavior change. I'm pretty sure composite:from-model does not index currently. I think this should probably default to false.

Good catch. That should default false.

stbrody · 2023-12-05T21:47:36Z

website/docs/api/commands/cli.composite.md

@@ -46,6 +46,10 @@ Create an encoded composite definition from GraphQL [Composite Schema](https://d

 You can find a detailed guide on the creation of Composites [here](https://developers.ceramic.network/docs/composedb/guides/data-modeling/composites)

+If updating your composite by specifying additional fields to filter on using the `createIndex` directive, run this 
+command with `--no-deploy`. Your GraphQL definition will still be updated, but Ceramic will not attempt to re-index your 


will not attempt to re-index your composite

will not attempt to re-index the Models in your composite

stbrody · 2023-12-05T21:49:30Z

website/docs/api/commands/cli.composite.md

@@ -117,6 +123,12 @@ available on the Ceramic Node that yor DApp connects to. You can find a detailed
 guide on Composites' deployment
 [here](https://developers.ceramic.network/docs/composedb/guides/data-modeling/composites#deploying-composites)

+If updating your composite to add additional query fields, do not run this command.


Did the language here get updated? I like the language you used for the create command above (mod one small suggestion), can we re-use that?

qbig · 2023-12-07T13:09:04Z

Just want to add some context here https://forum.ceramic.network/t/devtools-composite-frommodels-does-not-support-filter/1304

qbig · 2023-12-07T13:15:07Z

@PaulLeCam @stbrody The gist is we want to enable 2 workflows:

define models/composite from scratch & deploy and index them (pretty clear)
some other devs discover this model and would like to use it in their own application and as a result want to index these models and create new index definitions/composite (I think 2 is the confusing part)

stbrody

lgtm

PaulLeCam · 2023-12-13T08:10:21Z

packages/devtools-node/src/fs.ts

+export async function createComposite(
+  ceramic: CeramicClient,
+  path: PathInput,
+  deploy: boolean,


That's a breaking API change, please set a default value or bump to v0.7 with docs updates.

PaulLeCam · 2023-12-13T08:11:52Z

packages/devtools-node/src/fs.ts

@@ -36,12 +40,12 @@ export async function createComposite(ceramic: CeramicClient, path: PathInput):
 export async function readEncodedComposite(
  ceramic: CeramicClient | string,
  path: PathInput,
-  index?: boolean,
+  deploy: boolean,


This is misleading, reading from JSON always deploys the models to the node anyway so only the indexing request is optional, please keep as-is.

It doesn't though. Composite create is the only command that creates (deploys) models to the node. readEncodedComposite deploys in the same manner that the deploy command deploys. For consistency between commands and SDK, everything that deploys (indexes) now has a deploy parameter.

PaulLeCam · 2023-12-13T08:12:24Z

packages/devtools/src/composite.ts

@@ -236,7 +236,7 @@ export type FromModelsParams = CompositeOptions & {
   * Whether to add the Models to the index or not. If `true`, the Ceramic instance must be
   * authenticated with an admin DID. Defaults to `false`.
   */
-  index?: boolean
+  index: boolean


Please keep this optional.

dbcfd requested a review from stbrody November 15, 2023 21:55

stbrody reviewed Nov 15, 2023

View reviewed changes

packages/cli/src/commands/composite/create.ts Outdated Show resolved Hide resolved

packages/cli/src/commands/composite/create.ts Outdated Show resolved Hide resolved

packages/cli/src/commands/composite/create.ts Outdated Show resolved Hide resolved

stbrody requested a review from PaulLeCam November 15, 2023 22:02

PaulLeCam reviewed Nov 16, 2023

View reviewed changes

dbcfd force-pushed the feat/reload-model branch from 75c74b1 to 5194792 Compare November 16, 2023 21:13

dbcfd requested review from PaulLeCam and stbrody November 17, 2023 03:59

PaulLeCam suggested changes Nov 17, 2023

View reviewed changes

packages/cli/src/commands/composite/create.ts Outdated Show resolved Hide resolved

packages/cli/src/commands/composite/deploy.ts Outdated Show resolved Hide resolved

stbrody reviewed Nov 20, 2023

View reviewed changes

packages/cli/src/commands/composite/deploy.ts Outdated Show resolved Hide resolved

website/docs/api/commands/cli.composite.md Outdated Show resolved Hide resolved

dbcfd requested a review from PaulLeCam November 27, 2023 15:14

PaulLeCam suggested changes Nov 27, 2023

View reviewed changes

packages/cli/src/commands/composite/create.ts Outdated Show resolved Hide resolved

packages/cli/src/commands/composite/deploy.ts Outdated Show resolved Hide resolved

dbcfd requested a review from PaulLeCam November 27, 2023 17:54

dbcfd requested a review from stbrody November 30, 2023 20:39

stbrody requested changes Nov 30, 2023

View reviewed changes

dbcfd added 2 commits December 5, 2023 10:11

feat: add flag to deploy composite on creation

16b0ecf

Adds a flag to deploy composite when creating, which is defaulted to false. Additionally adds a flag to disable or enable indexing when deploying a composite.

review changes

2e41e5e

dbcfd force-pushed the feat/reload-model branch from 71292dd to 2e41e5e Compare December 5, 2023 18:42

dbcfd requested a review from stbrody December 5, 2023 19:29

stbrody reviewed Dec 5, 2023

View reviewed changes

changes based on review

dce5745

dbcfd requested a review from stbrody December 11, 2023 15:08

stbrody previously approved these changes Dec 12, 2023

View reviewed changes

PaulLeCam suggested changes Dec 13, 2023

View reviewed changes

changes based on review

946b19b

dbcfd dismissed stbrody’s stale review via 946b19b December 15, 2023 00:41

dbcfd requested a review from PaulLeCam December 15, 2023 03:49

m0ar mentioned this pull request Jan 12, 2024

Temporarily disable query indexes as bug workaround desci-labs/desci-codex#33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose flag to determine whether a composite is indexed #196

feat: expose flag to determine whether a composite is indexed #196

dbcfd commented Nov 15, 2023

stbrody left a comment

PaulLeCam left a comment

stbrody left a comment

PaulLeCam left a comment

dbcfd commented Nov 27, 2023

stbrody Nov 30, 2023

stbrody Nov 30, 2023

dbcfd Dec 4, 2023

stbrody Dec 5, 2023

stbrody Nov 30, 2023

stbrody Dec 5, 2023

stbrody Dec 5, 2023

stbrody Dec 5, 2023

dbcfd Dec 6, 2023

stbrody Dec 5, 2023

stbrody Dec 5, 2023

qbig commented Dec 7, 2023

qbig commented Dec 7, 2023

stbrody left a comment

PaulLeCam Dec 13, 2023

PaulLeCam Dec 13, 2023 •

edited

Loading

dbcfd Dec 14, 2023

PaulLeCam Dec 13, 2023

feat: expose flag to determine whether a composite is indexed #196

Are you sure you want to change the base?

feat: expose flag to determine whether a composite is indexed #196

Conversation

dbcfd commented Nov 15, 2023

stbrody left a comment

Choose a reason for hiding this comment

PaulLeCam left a comment

Choose a reason for hiding this comment

stbrody left a comment

Choose a reason for hiding this comment

PaulLeCam left a comment

Choose a reason for hiding this comment

dbcfd commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbig commented Dec 7, 2023

qbig commented Dec 7, 2023

stbrody left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulLeCam Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulLeCam Dec 13, 2023 •

edited

Loading