DNS flow refactors

[ghost]

I'd like to start a discussion about this file's TS declarations, because I'm quite certain it's wrong: https://github.com/DevelopingSpace/starchart/blob/main/app/queues/dns/dns-flow.server.ts First, we are trying to use JobRecord for absolutely everything. Basically all the workers and exported flow initiator functions all use this, and we are trying to twist it to suite the specific use-case. This causes some very hard to follow code, and inappropriate use. I.e., Line 17, name refers to a subdomain only (which was a breaking change, but let's not get bogged down) In Line 18, using name (subdomain) we create fullRecordName which is an FQDN In line 24, we pass this FQDN down as name, so in line 17 name was a subdomain, in line 24 name is FQDN and they are both using the same TS interface. This is wrong.

In my humble opinion:

• Each worker should be it's own file and should export an interface, of what it exactly expects as input params (data)
• dns-flow.server.ts should import those interfaces and use the correct interface for each data attribute in the flow generators
• There also should be a separate TS interface/type for each flow initiator. It can reuse the worker's interface, but should be specific.
• We should start using subdomain and fqdn atribute and variable names to signify what we are storing in it, name is not specific enough (see above issue)
• We need a way to pass down either FQDN or subdomain to the appropriate flow initiators, flow initiators should generate the fqdn if subdomain was provided. This is needed for the ACME integration (and probably other later code as well)
• Input validation should be part of the flow initiator, not the worker. This way we can synchronously return an error to the caller, and it's not the subsequent async job that's going to fail

[Genne23v]

Each worker should be it's own file and should export an interface, of what it exactly expects as input params (data)

@dadolhay I can split dns-flow.server.ts into several files. But the current workers has a benefit by having it all in one file as it's designed to reduce similar code. This is one example.

      switch (workType) {
        case 'create':
          return createRecord(username, type, name.toLowerCase(), value);

        case 'update':
          return upsertRecord(username, type, name.toLowerCase(), value);

        case 'delete':
          return deleteRecord(username, type, name.toLowerCase(), value);

        default:
          throw new UnrecoverableError(`Invalid work type: ${workType}`);
      }

I can define multiple interfaces here. Should we still have this code in separate files?

[humphd]

@Genne23v if it's not convenient to break the file up, don't bother. I just suggested it because our other code does it (for consistency). But it's not critical. Do what's best for the code.

[Genne23v]

@Genne23v if it's not convenient to break the file up, don't bother. I just suggested it because our other code does it (for consistency). But it's not critical. Do what's best for the code.

Thanks for the feedback. I just wanted to make sure that I'm not missing something valuable. I will come up with what I think is the best.

[ghost]

@Genne23v I meant to break up ./dns-record-worker.server. As far as I can see, the switch (workType) part is already repeated in the relevant workers, and I can't see too much reuse between them. IMO, this would help later in correctly structuring the code also. But as always, the decision is yours, I don't want to force my own coding style on anyone.

[ghost]

Just to share my thinking; I usually try to break out stuff that are loosely connected if the file size is > 100 lines. Not always possible but I found that to be a good practice anyways.

[Genne23v]

Thank you for the advice. I don't see much benefit to break the workers at the moment probably because I'm not able to foresee what should be coming next. Before wring more code, I will think more about how it should be handled for better maintainability as well as minimizing the code.