CLI: Support kafka trigger type

[midigofrank]

Short Description

This pull request adds kafka trigger type.

Fixes #796

Implementation Details

• The structure maintains the structure as it is in lightning
• There is a validation that uses regex to ensure that the host is in the form hostname:number

QA Notes

List any considerations/cases/advice for testing/QA here.

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

• Code generation (copilot but not intellisense)
• Learning or fact checking
• Strategy / design
• Optimisation / refactoring
• Translation / spellchecking / doc gen
• Other
• I have not used AI

You can read more details in our Responsible AI Policy

[josephjclark]

Hi @midigofrank, I'm recommending a couple of refactors because some of the code is getting stretched very thin here.

Is there anyway we can add a test on this? We should be able to at least add unit tests on transformSpecKafkaHost

[josephjclark]

This is getting way to complicated and I'm going to insist on a refactor here :(

I've just scribbled this and haven't tested it, but something along these lines would be a lot more readable and scalable:

 if (specTrigger && !stateTrigger) {
    const trigger = {
      id: crypto.randomUUID(),
      ...pickKeys(specTrigger, ['type', 'enabled']),
    };
    
    if (specTrigger.type === 'cron') {
      trigger.cron_expression = specTrigger.cron_expression
    } else if (specTrigger.type === 'kafka') {
      trigger.kafka_configuration = {
        ...specTrigger.kafka_configuration,
        hosts: transformSpecKafkaHost(
          specTrigger.kafka_configuration?.hosts ?? []
        ),
      },
    }

    return [triggerKey, trigger]
  }

[midigofrank]

Hmm, yeah I agree.

[josephjclark]

Same again - this block needs to be refactored for readability

[josephjclark]

I would default the argument of transformSpecKafkaHost inside the function declaration, rather than passing an empty array here:

function transformSpecKafkaHost(hosts: string[] = []): StateKafkaHost[] { ... }

[josephjclark]

I'm a little confused by . and - at the end of the host regex here?

. means any character, so it kind of means the first pad means "match any character" and renders the rest obsolete.

I'm not sure why we need to support -

[josephjclark]

Hey was this an AI generated regex by any chance?

[josephjclark]

Another pattern I see for this sort of thing is to use the URL object to parse a url string, and then test for host and port key.

I'm not convinced this approach is better than regex, but it's definitely better than a bad regex 😅

[josephjclark]

Huh and maybe a dumb question - I'm nitpicking and anyway I don't know much about kafka... but does it definitely need a port in the URL? It can't run off a default port?

[midigofrank]

Yeah, it is AI generated 😅 . I did a couple of tests and it was able to capture all the bad cases I could think of.

I tried using the URL object but it proved troublesome because the protocol is missing from the required input. new URL('localhost:9092') parses localhost as the protocol. And yes, you MUST specify the port when connecting to a kafka cluster

[midigofrank]

Let me see if I can simplify this. Maybe we don't even need a regex, the intention is to make sure that there is a hostname:port_number in the string. AI decided to take it a notch higher by trying to ensure that the hostname is a valid ip address

[josephjclark]

Ah, then probably the AI wanted to escape the .

Good practice to turn those tests into unit tests - if only against the regex itself - so that readers can see what sort of patterns work (and maybe don't work!)

I think a simple regex or new URL is totally fine, so long as there are a few unit tests. Don't need to spend too long on it!