database changes to include jobs#68
Conversation
| # They will interrogate the database and see what runs already exist. | ||
| # Any runs that need running, will be. | ||
|
|
||
| jobId: Random_Job_ID |
There was a problem hiding this comment.
Let's:
- Find a broader top-level for this. We will probably eventually need to have a few things passed from FaaS to jenkins-sdk. Let's not have loads of new top-level keys for that.
- Probably leave it out of this particular file. It's just confusing and users won't know whether they are meant to supply something - plus all daily FIT/PERF runs will get put in the same job? It can just be added only by FaaS.
There was a problem hiding this comment.
It can just be added only by FaaS.
yep that makes sense will make the change
There was a problem hiding this comment.
What does this look like now - is it?
jc:
jobId:
Can we use something more descriptive and intuitive than jc (is that job config?). How about:
job:
jobId:
| // JSONB indexes | ||
| execute(sql, env, "CREATE INDEX IF NOT EXISTS idx_runs_params ON runs USING gin (params)") | ||
| execute(sql, env, "CREATE INDEX IF NOT EXISTS idx_runs_events_params ON run_events USING gin (params)") | ||
| // New Jobs table to hold an overlying job for grouping runs |
There was a problem hiding this comment.
Please slot this into the existing code a little better. This is in the middle of creating indexes. Create a separate "// FIT-as-a-Service" section maybe.
| + "id uuid PRIMARY KEY, " | ||
| + "datetime timestamp NOT NULL DEFAULT now(), " | ||
| + "config jsonb, " | ||
| + "tags TEXT[] DEFAULT NULL" |
There was a problem hiding this comment.
I think we should just keep metadata as JSON e.g. in the config blob. I appreciate that won't be a universally agreed idea, and I'm open to an argument to have it as a separate field like this. But I noticed in a recent share that you have it in both places, which definitely isn't a good idea.
There was a problem hiding this comment.
I didn't want to do that since fro the autocomplete feature fro tags in FAAS the tags need to be read and displayed.
So adding them into the jsonb would just be additional processing, also would not be able to make the index on the tags(which was the whole point of adding tags imo)
There was a problem hiding this comment.
You can index anything in JSON - look up JSONB GIN indexes.
I guess I don't feel so strongly on this, but please make sure they're not both in a separate column and the config blob. We don't want values to be duplicated.
| + "tags TEXT[] DEFAULT NULL" | ||
| + ")"); | ||
| // Join table to link jobs with runs | ||
| execute(sql, env, "CREATE TABLE IF NOT EXISTS job_runs (" |
There was a problem hiding this comment.
Let's stick with the same table naming convention as the existing join table.
|
Thanks for keeping this a nice clean change. It looks pretty good to me, just a couple of notes. |
2 participants
- Job is a wrapper around all tasks under 1 job-config file