Allow users to present String and Integer params in a ManifestSpec

[Berlioz]

pre-emptive nit: if the user provides no params, should ManifestSpec.params be an empty array, or undefined?

[taeold]

nit* If we are going to change the name, should we also change the name of type ParamValueType to type ParamType?

[taeold]

nit* I think our guidance will be to use capital letters when defining params. So FOO instead of foo here?

[inlined]

This is a really interesting use case where even a team member got tripped up on our proposal. What should we do?

1. Throw a runtime error if the param name has invalid characters
2. Try to use the type system to require that this an uppercase string
3. Magically transform the given name into valid characters (in this case FOO)

[taeold]

My vote would be for 1 (specifically runtime error that's picked up and nicely presented by the CLI as deploy error) and 2.

I feel that I've always been against magic transformation like 3 though I'm slowly turning myself around it (e.g. magically transform function name for v2)

[inlined]

This is a really interesting use case where even a team member got tripped up on our proposal. What should we do?

1. Throw a runtime error if the param name has invalid characters
2. Try to use the type system to require that this an uppercase string
3. Magically transform the given name into valid characters (in this case FOO)

[inlined]

Nit: you don't need to send a 200 status; it's inferred if you just say send

[inlined]

@mbleigh It still feels wrong to me that a param has a discriminating union field and input doesn't.

Why is it

{
  name: "FOO",
  type: "string",
  input: {
    textInput: {}
  }
}

and not

{
  name: "FOO",
  type: "string",
  input: {
    type: "text",
  }
}

or even

{
  stringParam: {
    name: "FOO",
    type: "string",
    input: {
      textInput: {}
    }
  }
}

It seems like those would be more ideologically consistent.

[mbleigh]

Because one is defining a small enumerated set of types to which a string value can be cast, and the other is defining an unbounded set of configuration and validation hints for client UIs.

The main reason I care is because I don't want us to end up with a giant grab-bag of fields with insanely overcomplicated "if type is X then fields A, B, D, and F are applicable. if type is Y then fields A, D, F, G, and K are applicable".

The cast type has no additional configuration, so it works fine as a simple enum. Likewise if the input type had no additional configuration, it would be fine as a simple enum. But we aim to have many different types of inputs (checkbox, radio group, dropdown, input, text field, resource picker, secret picker, more as we need) and each of those will have some amount of configuration.

I look at a oneof as basically being an "enum with configuration", which is why I don't find this API inconsistent.

[inlined]

I like Scala's term for "enum with configuration"--case class.

I see some of your argument, though the last example (where each is a oneof message) follows the argument even further. In the proposal, all input types can be used with all parameter types, but this isn't true. As I understand the current proposal, multiselect input can only be used with a list param (which can only use a multiselect input I think). And a text input can only use certain validators depending on their param type. While the last format makes my stomach churn instinctively, it has the benefit that the type system only allows the right input to be picked with the right param type.

[mbleigh]

Actually I don't think there should be strict couplings of input type and param type. If someone prefers a comma separated string instead of a list for a multi select that works fine for me. If someone wants you to type the string "true" and cast it to bool instead of a checkbox, fine by me. I don't think the two types need be correlated beyond the fact that they usually will tend toward command expressions.

…

On Tue, Aug 9, 2022, 7:28 PM Thomas Bouldin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In spec/runtime/loader.spec.ts <#1166 (comment)> : > @@ -310,6 +310,11 @@ describe('loadStack', () => { }, }, }, + { + name: 'has params', + modulePath: './spec/fixtures/sources/commonjs-params', + expected: { ...expected, params: [{ name: 'FOO', type: 'string' }] }, I like Scala's term for "enum with configuration"--case class. I see some of your argument, though the last example (where each is a oneof message) follows the argument even further. In the proposal, all input types can be used with all parameter types, but this isn't true. As I understand the current proposal, multiselect input can only be used with a list param (which can only use a multiselect input I think). And a text input can only use certain validators depending on their param type. While the last format makes my stomach churn instinctively, it has the benefit that the type system only allows the right input to be picked with the right param type. — Reply to this email directly, view it on GitHub <#1166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAH7VDJL4DDMILMNJ4B73VYMHVFANCNFSM523QSLFQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[inlined]

That implies we need to support a cross product of features rather than using the type system to restrict what we allow. I'd need to be convinced.

[mbleigh]

I think of it as looser coupling - inputs always result in strings, param types always try to coerce string values into their specified types. Neither has to know or care about what the other does. We don't need to guard it with the type system because input is purely used for client hinting, it has no semantics.

[inlined]

List params are still a wart since you need to know your string and your delimiter, but one edge case doesn't sink a ship

[mbleigh]

Oh I was assuming we wouldn't support customized delimiters, just use commas and maybe allow escaping if we get feedback about needing values with commas 🤷