From b64bbaa0bb8a4cb6f72b3beec7ad3662fbf0923c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Budai?= Date: Wed, 12 Feb 2020 15:18:47 +0100 Subject: [PATCH] api/jobqueue: move build id to url Imho it makes more sense from REST perspective. Also, in the future there will be ROUTE for uploading image to image build. As it's not a good idea transport file inside JSON, all the parameters (compose id and image build id) need to be inside the URL. Therefore for the sake of consistency, all these routes should have compose id and image build id in the URL. There is another solution to embedding multiple values inside http body which allows file transport - multipart/form-data. I think using form-data is worth when doing more complex stuff, for our usecase transporting all the metadata in the URL is more appropriate solution. --- cmd/osbuild-worker/main.go | 5 +++-- internal/jobqueue/api.go | 14 +++++++++++--- internal/jobqueue/api_test.go | 10 +++++----- internal/jobqueue/job.go | 5 ++--- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/cmd/osbuild-worker/main.go b/cmd/osbuild-worker/main.go index 6a2ed2450..03e1de062 100644 --- a/cmd/osbuild-worker/main.go +++ b/cmd/osbuild-worker/main.go @@ -56,8 +56,9 @@ func (c *ComposerClient) AddJob() (*jobqueue.Job, error) { func (c *ComposerClient) UpdateJob(job *jobqueue.Job, status common.ImageBuildState, result *common.ComposeResult) error { var b bytes.Buffer - json.NewEncoder(&b).Encode(&jobqueue.JobStatus{status, job.ImageBuildID, result}) - req, err := http.NewRequest("PATCH", "http://localhost/job-queue/v1/jobs/"+job.ID.String(), &b) + json.NewEncoder(&b).Encode(&jobqueue.JobStatus{status, result}) + url := fmt.Sprintf("http://localhost/job-queue/v1/jobs/%s/builds/%d", job.ID.String(), job.ImageBuildID) + req, err := http.NewRequest("PATCH", url, &b) if err != nil { return err } diff --git a/internal/jobqueue/api.go b/internal/jobqueue/api.go index 7077a6974..a5cf94c66 100644 --- a/internal/jobqueue/api.go +++ b/internal/jobqueue/api.go @@ -5,6 +5,7 @@ import ( "log" "net" "net/http" + "strconv" "github.com/osbuild/osbuild-composer/internal/store" @@ -31,7 +32,7 @@ func New(logger *log.Logger, store *store.Store) *API { api.router.NotFound = http.HandlerFunc(notFoundHandler) api.router.POST("/job-queue/v1/jobs", api.addJobHandler) - api.router.PATCH("/job-queue/v1/jobs/:id", api.updateJobHandler) + api.router.PATCH("/job-queue/v1/jobs/:job_id/builds/:build_id", api.updateJobHandler) return api } @@ -114,12 +115,19 @@ func (api *API) updateJobHandler(writer http.ResponseWriter, request *http.Reque return } - id, err := uuid.Parse(params.ByName("id")) + id, err := uuid.Parse(params.ByName("job_id")) if err != nil { statusResponseError(writer, http.StatusBadRequest, "invalid compose id: "+err.Error()) return } + imageBuildId, err := strconv.Atoi(params.ByName("build_id")) + + if err != nil { + statusResponseError(writer, http.StatusBadRequest, "invalid image build id: "+err.Error()) + return + } + var body JobStatus err = json.NewDecoder(request.Body).Decode(&body) if err != nil { @@ -127,7 +135,7 @@ func (api *API) updateJobHandler(writer http.ResponseWriter, request *http.Reque return } - err = api.store.UpdateImageBuildInCompose(id, body.ImageBuildID, body.Status, body.Result) + err = api.store.UpdateImageBuildInCompose(id, imageBuildId, body.Status, body.Result) if err != nil { switch err.(type) { case *store.NotFoundError: diff --git a/internal/jobqueue/api_test.go b/internal/jobqueue/api_test.go index cb23293ee..39787be11 100644 --- a/internal/jobqueue/api_test.go +++ b/internal/jobqueue/api_test.go @@ -25,11 +25,11 @@ func TestBasic(t *testing.T) { // Create job with invalid body {"POST", "/job-queue/v1/jobs", ``, http.StatusBadRequest, `invalid request: EOF`}, // Update job with invalid ID - {"PATCH", "/job-queue/v1/jobs/foo", `{"status":"RUNNING"}`, http.StatusBadRequest, `invalid compose id: invalid UUID length: 3`}, + {"PATCH", "/job-queue/v1/jobs/foo/builds/0", `{"status":"RUNNING"}`, http.StatusBadRequest, `invalid compose id: invalid UUID length: 3`}, // Update job that does not exist, with invalid body - {"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", ``, http.StatusBadRequest, `invalid status: EOF`}, + {"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/builds/0", ``, http.StatusBadRequest, `invalid status: EOF`}, // Update job that does not exist - {"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", `{"image_build_id": 0, "status":"RUNNING"}`, http.StatusNotFound, `compose does not exist`}, + {"PATCH", "/job-queue/v1/jobs/aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa/builds/0", `{"status":"RUNNING"}`, http.StatusNotFound, `compose does not exist`}, } for _, c := range cases { @@ -72,12 +72,12 @@ func testUpdateTransition(t *testing.T, from, to string, expectedStatus int, exp if from != "WAITING" { test.SendHTTP(api, false, "POST", "/job-queue/v1/jobs", `{}`) if from != "RUNNING" { - test.SendHTTP(api, false, "PATCH", "/job-queue/v1/jobs/ffffffff-ffff-ffff-ffff-ffffffffffff", `{"status":"`+from+`"}`) + test.SendHTTP(api, false, "PATCH", "/job-queue/v1/jobs/ffffffff-ffff-ffff-ffff-ffffffffffff/builds/0", `{"status":"`+from+`"}`) } } } - test.TestNonJsonRoute(t, api, false, "PATCH", "/job-queue/v1/jobs/ffffffff-ffff-ffff-ffff-ffffffffffff", `{"status":"`+to+`"}`, expectedStatus, expectedResponse) + test.TestNonJsonRoute(t, api, false, "PATCH", "/job-queue/v1/jobs/ffffffff-ffff-ffff-ffff-ffffffffffff/builds/0", `{"status":"`+to+`"}`, expectedStatus, expectedResponse) } func TestUpdate(t *testing.T) { diff --git a/internal/jobqueue/job.go b/internal/jobqueue/job.go index a9dfb4c17..2aa4d676f 100644 --- a/internal/jobqueue/job.go +++ b/internal/jobqueue/job.go @@ -26,9 +26,8 @@ type Job struct { } type JobStatus struct { - Status common.ImageBuildState `json:"status"` - ImageBuildID int `json:"image_build_id"` - Result *common.ComposeResult `json:"result"` + Status common.ImageBuildState `json:"status"` + Result *common.ComposeResult `json:"result"` } type TargetsError struct {