-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mrc 5585 submit endpoint #6
Changes from 19 commits
87bfacb
96c68f3
570ae37
01e9bbc
5afb8c7
e0956ac
4b314d8
851f0c1
dc5ea27
dea652a
319b4c6
4cd8e9b
2b50e2d
b7f4337
3885f80
018cfa9
e15b628
5549fcb
5e533b6
fca17c8
104f580
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,27 @@ | ||||||||||||
{ | ||||||||||||
"$schema": "http://json-schema.org/draft-07/schema#", | ||||||||||||
"type": "object", | ||||||||||||
"properties": { | ||||||||||||
"name": { | ||||||||||||
"type": "string" | ||||||||||||
}, | ||||||||||||
"branch": { | ||||||||||||
"type": "string" | ||||||||||||
}, | ||||||||||||
"hash": { | ||||||||||||
"type": "string" | ||||||||||||
}, | ||||||||||||
"parameters": { | ||||||||||||
"oneOf": [ | ||||||||||||
{ "type": "null" }, | ||||||||||||
{ | ||||||||||||
"type": "object", | ||||||||||||
"properties": {}, | ||||||||||||
"additionalProperties": true | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
we can be stricter here, and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||||||||
} | ||||||||||||
] | ||||||||||||
} | ||||||||||||
}, | ||||||||||||
"required": ["name", "branch", "hash", "parameters"], | ||||||||||||
"additionalProperties": false | ||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"type": "object", | ||
"properties": { | ||
"job_id": { | ||
"type": "string" | ||
} | ||
}, | ||
"required": ["job_id"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason not to return as a string rather than an object? if an object any reason not to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no reason other than i was speculating that we may want to add other things that return from this endpoint, and having an object returned means an easier future refactor in kotlin if another field is needed yep this can definitely be camelcase job_id was only used because I believe the sequence diagram uses job id and that percolated through to the tickets, i can change it to task! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have changed to |
||
"additionalProperties": false | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
orderly2::orderly_artefact("Some data", "data.rds") | ||
orderly2::orderly_artefact(description = "Some data", "data.rds") | ||
d <- data.frame(a = 1:10, x = runif(10), y = 1:10 + runif(10)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, this is for the greater good... |
||
saveRDS(d, "data.rds") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have both branch and hash, and not ref? Probably this is a good idea though tbh
What is the interpretation of branch names? Do we add main/ to them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the runner checks out to that branch and resets head to the hash, we would have a different behaviour if we used a ref right? would we not get a detached head if someone specified just a hash
there will be some extra work in the UI if we want people to be able to specify a commit hash on a branch instead of just a commit hash but that can come later
branch names are just what we do a git checkout to before resetting to the specified hash, im not sure what you mean by adding
main/
to them but we dont do anything like that, just pass branchname into git checkout via gert