List Params support in CEL #5137

Berlioz · 2022-10-17T23:19:46Z

"This creaky regular expression based hack of an interpreter for our limited CEL subset can't possibly handle more features!," said Frog, as he built another feature on top of it

codecov-commenter · 2022-10-19T21:32:49Z

Codecov Report

Base: 56.41% // Head: 56.30% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (65c5635) compared to base (77e6e6d).
Patch coverage: 40.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5137      +/-   ##
==========================================
- Coverage   56.41%   56.30%   -0.12%     
==========================================
  Files         315      315              
  Lines       21150    21282     +132     
  Branches     4288     4338      +50     
==========================================
+ Hits        11932    11982      +50     
- Misses       8193     8263      +70     
- Partials     1025     1037      +12

Impacted Files	Coverage Δ
src/deploy/functions/prepare.ts	`31.42% <0.00%> (-0.16%)`	⬇️
...rc/deploy/functions/runtimes/discovery/v1alpha1.ts	`76.62% <ø> (ø)`
src/deploy/functions/params.ts	`31.95% <15.38%> (-5.55%)`	⬇️
src/deploy/functions/build.ts	`61.62% <40.00%> (-0.51%)`	⬇️
src/deploy/functions/runtimes/discovery/parsing.ts	`74.13% <60.00%> (-1.79%)`	⬇️
src/deploy/functions/cel.ts	`79.62% <71.66%> (-2.41%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

inlined

The winds are blowing in the direction of making lists be delimited strings rather than JSON expressions. This would make it easier to represent in GUI dialogs and reads easier in .env files.

inlined · 2022-11-01T18:58:28Z

src/deploy/functions/build.ts

 export type Field<T extends string | number | boolean> = T | Expression<T> | null;
+export type FieldList = Expression<string[]> | (string | Expression<string>)[] | null;


I do wonder if we can start by assuming you have either a string[] or Expression<string[]>. Array<string | Expression> may be hard to represent in Proto and it may not be a hard blocker for people to not be able to hard code only parts of a list.

inlined · 2022-11-01T18:59:12Z

src/deploy/functions/build.ts

 export type Field<T extends string | number | boolean> = T | Expression<T> | null;
+export type FieldList = Expression<string[]> | (string | Expression<string>)[] | null;


Also, nit picking, if it's a FieldList it sounds like a list of fields. A ListField sounds like a field that is a list, which personally sounds more accurate to my ears. NBD if you disagree though.

inlined · 2022-11-01T19:09:55Z

src/deploy/functions/params.ts

+      (res: string[]) => res
+    );
+  } else if (isTextInput(param.input)) {
+    prompt = `Enter a list of strings for ${param.label || param.name}:`;


This will probably get more complex if/when we transition to inputting with a delimiter. If your delimiter is newline, you probably have to accept double newlines as the way you end your prompt. IDK if the prompt library supports that, so we might, for now stick to just comma delimiters.

inlined · 2022-11-01T19:15:20Z

src/deploy/functions/params.ts

@@ -584,6 +688,10 @@ async function promptResourceString(
 }

 type retryInput = { message: string };
+function shouldRetry(obj: any): obj is retryInput {
+  return (obj as retryInput).message !== undefined;


Do you also need to verify that obj is indeed an object? It looks like the previous code this is replacing used to check for that.

Good catch.

inlined · 2022-11-01T19:16:30Z

src/deploy/functions/params.ts

+  resolvedDefault: T[] | undefined,
+  converter: (res: string[]) => T[] | retryInput
+): Promise<T[]> {
+  const response = await promptOnce({


In firebase init we explain to people that they should use space to select a list item and return to choose the selection. We should probably add that to the prompt here. Similarly, when using a text input for list items, we should explain the delimiter usage.

inlined · 2022-11-01T19:17:50Z

src/deploy/functions/runtimes/discovery/parsing.ts

@@ -106,6 +107,14 @@ export function assertKeyTypes<T extends object>(
      }
      continue;
    }
+    if (schemaType === "List") {


Technically you've defined "List?" as a type. Do you want to check for value === null first or is that case already handled?

It looks to me like the conditional checks immediately above that one cover this case, unless I'm badly misinterpreting your comment.

inlined · 2022-11-01T19:19:28Z

src/test/deploy/functions/cel.spec.ts

+      expect(
+        resolveExpression(
+          "string[]",
+          '{{ params.FOO == params.FOO ? [ "foo", params.BAR, {{ params.BAR }} ] : [] }}',


Shouldn't the last field be "{{ params.BAR }}" (with the quotes)

src/deploy/functions/cel.ts

taeold · 2022-11-09T17:38:43Z

src/deploy/functions/cel.ts

+    // by the preprocessLists() invocation at the beginning of CEL resolution
+    const parsed = JSON.parse(value);
+    if (!Array.isArray(parsed)) {
+      throw new ExprParseError("");


Can we include more descriptive parser error message?

taeold · 2022-11-09T17:46:14Z

src/deploy/functions/params.ts

+      (res: string[]) => res
+    );
+  } else if (isTextInput(param.input)) {
+    prompt = `Enter a list of strings for ${param.label || param.name}:`;


can we expand the prompt to help instruct users how they can input the delimeter-separated list input?

src/deploy/functions/params.ts

taeold · 2022-12-02T19:13:14Z

src/deploy/functions/params.ts

+  resolvedDefault: T[] | undefined,
+  converter: (res: string[]) => T[] | retryInput
+): Promise<T[]> {
+  const response = await promptOnce({


Co-authored-by: Daniel Lee <danielylee@google.com>

Berlioz added 2 commits October 17, 2022 16:16

first commit: param lists

a1a2983

love too have initial trivial test case be broken

ace8a2b

Berlioz requested review from inlined and taeold October 17, 2022 23:19

Berlioz added 2 commits October 17, 2022 23:59

maybe fix it so it actually works

7c2dbd2

handle list comparisons

faa1eeb

Berlioz changed the title ~~WIP: List Params~~ List Params support in CEL Oct 19, 2022

Berlioz added 3 commits October 20, 2022 13:25

Merge branch 'master' into list_params

42e6334

actually support a real list param

af42549

um, placeholder input methods?

5bd2940

inlined reviewed Nov 1, 2022

View reviewed changes

Berlioz added 6 commits November 1, 2022 12:47

.env lists as comma seperated strings instead of serialized objects

d628d9f

-what if the previous commit but actually working

1c6df4e

smaller style fixes

cf66cd0

Merge branch 'master' into list_params

0fba2ef

some work on input methods

5cf4480

Merge remote-tracking branch 'origin' into list_params

54dab9b

taeold approved these changes Nov 9, 2022

View reviewed changes

Berlioz added 4 commits November 15, 2022 22:28

Merge remote-tracking branch 'origin' into list_params

37ba78f

assorted changes per reviews

8782c04

eslint fix

bd77603

Merge remote-tracking branch 'origin' into list_params

655a546

taeold reviewed Dec 2, 2022

View reviewed changes

Berlioz and others added 6 commits December 18, 2022 21:02

Merge remote-tracking branch 'origin' into list_params

7807eb4

Update src/deploy/functions/cel.ts

afcf6f3

Co-authored-by: Daniel Lee <danielylee@google.com>

Update src/deploy/functions/params.ts

b2de4f6

Co-authored-by: Daniel Lee <danielylee@google.com>

Update src/deploy/functions/params.ts

c8aa2ef

Co-authored-by: Daniel Lee <danielylee@google.com>

Update src/deploy/functions/params.ts

8cc6f89

Co-authored-by: Daniel Lee <danielylee@google.com>

Update src/deploy/functions/params.ts

b54d489

Co-authored-by: Daniel Lee <danielylee@google.com>

Berlioz added 2 commits December 18, 2022 23:09

more error messages filled in

eb54228

eslint fix

65c5635

Berlioz merged commit e939a2b into master Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List Params support in CEL #5137

List Params support in CEL #5137

Berlioz commented Oct 17, 2022 •

edited

codecov-commenter commented Oct 19, 2022 •

edited

inlined left a comment

inlined Nov 1, 2022

inlined Nov 1, 2022

inlined Nov 1, 2022

inlined Nov 1, 2022

Berlioz Nov 1, 2022

inlined Nov 1, 2022

taeold Dec 2, 2022

inlined Nov 1, 2022

Berlioz Nov 1, 2022

inlined Nov 1, 2022

taeold Nov 9, 2022

taeold Nov 9, 2022

taeold Dec 2, 2022

		export type Field<T extends string \| number \| boolean> = T \| Expression<T> \| null;
		export type FieldList = Expression<string[]> \| (string \| Expression<string>)[] \| null;

List Params support in CEL #5137

List Params support in CEL #5137

Conversation

Berlioz commented Oct 17, 2022 • edited

codecov-commenter commented Oct 19, 2022 • edited

Codecov Report

inlined left a 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

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

Berlioz commented Oct 17, 2022 •

edited

codecov-commenter commented Oct 19, 2022 •

edited