Review & Simplify the Client side code

[sravankorumilli]

There is an observation around multiple conversions, lets analyse whether we can simplify this.

Job Yaml tranform to models.Job -> Job Yaml for persistence Job Yaml tranform to models.Job -> protobuf Job Model.

[irainia]

So far, the following is the observed relationship between job structure in various layers:

models.JobSpec (models/job.go)
  |
  |-- store.local.Job (store/local/job_spec_adapter.go)
  |-- store.postgres.Job (store/postgres/adapter.go)
  |-- [skip] api.proto.odpf.optimus.core.v1beta1.JobSpecification (api/handler/v1beta1/job_spec.go)

• conversion store.local.Job (store/local/job_spec_adapter.go) --> models.JobSpec (models/job.go)

This conversion happens when there's a need to convert local spec to model spec, which in general does as below:

• parsing start date and (if specified) also end date

  • we can use a more generic validator for this, such as go-playground's, which can be used multiple times

• standardizing dependency type

  • we can use default value

• http dependency validation

  • we can use one structure for this, and implement validator

• validating chosen hooks if it's installed or not

  • we can use a more generic validator

• convert mapslice config to config item from interfaces to strings

  • we can use a simpler struct directly, without having to use yaml.MapSlice or just use map[string]string

• note on prepareWindow function:

  • [possible bug] when trying to parse the remaining time on tryParsingInMonths function, what if the month is negative, isn't remaining time going to be moving it forward instead of backward?
  • error returned from the tryParsingInMonths is not used

• getting the concrete installed plugin implementation

• copy labels with the exact same value

• ToSpec function is required to be a method as it requires plugin to be set

  • if this function is to be made as independent function, we can just move the plugin as parameter, or directly access the plugin by calling the model package

• conversion models.JobSpec (models/job.go) --> store.local.Job (store/local/job_spec_adapter.go)

This conversion contains not much of sophisticated implementation when compared with the previous conversion. Generally, this process invovles:

• converting config to MapSlice, JobNotifier, and some other fields
• formatting the model to local format

Based on the points mentioned above, for the relationship between models.JobSpec (models/job.go) and store.local.Job (store/local/job_spec_adapter.go), suggestion for improvement:

• separating the responsibility of the function, from previously handling validation and conversion together, into separated validation and separated conversion

[sravankorumilli]

@irainia keep the validation in a single place makes sense, and we can verify the bug through some unit tests and create a separate bug card and fix the same.