Repository: osbuild/osbuild-composer
Pull request #412 (2020-03-27 22:51:44)

rcm: rework the structure of the compose requests
See the commits for more details.

teg: That was not a well formed sentence, sorry. We don't want a list of architectures and a separate list of image types. That would presumably mean that we want to build images for every combination, but we want to explicitly state which pairs to use.

Patch:
diff --git a/internal/rcm/api.go b/internal/rcm/api.go
index 202bd1ded..da4216776 100644
--- a/internal/rcm/api.go
+++ b/internal/rcm/api.go
@@ -8,7 +8,6 @@ import (
 	"log"
 	"net"
 	"net/http"
-	"strings"
 
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 
@@ -107,16 +106,23 @@ func (api *API) submit(writer http.ResponseWriter, request *http.Request, _ http
 		return
 	}
 
-	type Repository struct {
-		URL string `json:"url"`
+	type repository struct {
+		BaseURL    string `json:"baseurl,omitempty"`
+		Metalink   string `json:"metalink,omitempty"`
+		MirrorList string `json:"mirrorlist,omitempty"`
+		GPGKey     string `json:"gpgkey,omitempty"`
+	}
+
+	type imageBuild struct {
+		Distribution string       `json:"distribution"`
+		Architecture string       `json:"architecture"`
+		ImageType    string       `json:"image_type"`
+		Repositories []repository `json:"repositories"`
 	}
 
 	// JSON structure expected from the client
 	var composeRequest struct {
-		Distribution  string       `json:"distribution"`
-		ImageTypes    []string     `json:"image_types"`
-		Architectures []string     `json:"architectures"`
-		Repositories  []Repository `json:"repositories"`
+		ImageBuilds []imageBuild `json:"image_builds"`
 	}
 	// JSON structure with error message
 	var errorReason struct {
@@ -126,35 +132,27 @@ func (api *API) submit(writer http.ResponseWriter, request *http.Request, _ http
 	decoder := json.NewDecoder(request.Body)
 	decoder.DisallowUnknownFields()
 	err := decoder.Decode(&composeRequest)
-	if err != nil || len(composeRequest.Architectures) != 1 || len(composeRequest.ImageTypes) != 1 || len(composeRequest.Repositories) == 0 {
+	if err != nil {
 		writer.WriteHeader(http.StatusBadRequest)
-		errors := []string{}
+		err = json.NewEncoder(writer).Encode(err.Error())
 		if err != nil {
-			errors = append(errors, err.Error())
-		}
-		if len(composeRequest.ImageTypes) == 0 {
-			errors = append(errors, "input must specify an image type")
-		} else if len(composeRequest.ImageTypes) != 1 {
-			errors = append(errors, "multiple image types are not yet supported")
-		}
-		if len(composeRequest.Architectures) == 0 {
-			errors = append(errors, "input must specify an architecture")
-		} else if len(composeRequest.Architectures) != 1 {
-			errors = append(errors, "multiple architectures are not yet supported")
-		}
-		if len(composeRequest.Repositories) == 0 {
-			errors = append(errors, "input must specify repositories")
+			panic("Failed to write response")
 		}
-		errorReason.Error = strings.Join(errors, ", ")
-		err = json.NewEncoder(writer).Encode(errorReason)
+		return
+	}
+
+	if len(composeRequest.ImageBuilds) != 1 {
+		writer.WriteHeader(http.StatusBadRequest)
+		_, err := writer.Write([]byte("unsupported number of image builds"))
 		if err != nil {
-			// JSON encoding is clearly our fault.
-			panic("Failed to encode errors in RCM API. This is a bug.")
+			panic("Failed to write response")
 		}
 		return
 	}
 
-	distro := api.distros.GetDistro(composeRequest.Distribution)
+	buildRequest := composeRequest.ImageBuilds[0]
+
+	distro := api.distros.GetDistro(buildRequest.Distribution)
 	if distro == nil {
 		writer.WriteHeader(http.StatusBadRequest)
 		_, err := writer.Write([]byte("unknown distro"))
@@ -164,7 +162,7 @@ func (api *API) submit(writer http.ResponseWriter, request *http.Request, _ http
 		return
 	}
 
-	arch, err := distro.GetArch(composeRequest.Architectures[0])
+	arch, err := distro.GetArch(buildRequest.Architecture)
 	if err != nil {
 		writer.WriteHeader(http.StatusBadRequest)
 		_, err := writer.Write([]byte("unknown architecture for distro"))
@@ -174,7 +172,7 @@ func (api *API) submit(writer http.ResponseWriter, request *http.Request, _ http
 		return
 	}
 
-	imageType, err := arch.GetImageType(composeRequest.ImageTypes[0])
+	imageType, err := arch.GetImageType(buildRequest.ImageType)
 	if err != nil {
 		writer.WriteHeader(http.StatusBadRequest)
 		_, err := writer.Write([]byte("unknown image type for distro and architecture"))
@@ -184,14 +182,15 @@ func (api *API) submit(writer http.ResponseWriter, request *http.Request, _ http
 		return
 	}
 
-	// Create repo configurations from the URLs in the request. Use made up repo id and name, because
-	// we don't want to bother clients of this API with details like this
+	// Create repo configurations from the URLs in the request.
 	repoConfigs := []rpmmd.RepoConfig{}
-	for n, repo := range composeRequest.Repositories {
+	for n, repo := range buildRequest.Repositories {
 		repoConfigs = append(repoConfigs, rpmmd.RepoConfig{
-			Id:        fmt.Sprintf("repo-%d", n),
-			BaseURL:   repo.URL,
-			IgnoreSSL: false,
+			Id:         fmt.Sprintf("repo-%d", n),
+			BaseURL:    repo.BaseURL,
+			Metalink:   repo.Metalink,
+			MirrorList: repo.MirrorList,
+			GPGKey:     repo.GPGKey,
 		})
 	}
 


Test patch:
diff --git a/cmd/osbuild-rcm-tests/main_test.go b/cmd/osbuild-rcm-tests/main_test.go
index 110db1e1a..e8f686fa7 100644
--- a/cmd/osbuild-rcm-tests/main_test.go
+++ b/cmd/osbuild-rcm-tests/main_test.go
@@ -7,22 +7,31 @@ package main
 
 import (
 	"encoding/json"
-	"github.com/google/uuid"
-	"github.com/stretchr/testify/require"
 	"net/http"
 	"strings"
 	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
 )
 
 func TestRCM(t *testing.T) {
 	// This is the first request the user sends to osbuild.
 	submitBody := `
 		{
-			"distribution": "fedora-31",
-		 	"image_types": ["qcow2"], 
- 		 	"architectures":["x86_64"], 
-			"repositories": [
-				{"url": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"}
+			"image_builds":
+			[
+				{
+					"distribution": "fedora-31",
+					"architecture": "x86_64",
+					"image_type": "qcow2",
+					"repositories":
+					[
+						{
+							"baseurl": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
+						}
+					]
+				}
 			]
 		}
 	`
diff --git a/internal/rcm/api_test.go b/internal/rcm/api_test.go
index f5c4fb2bc..7f3a9597e 100644
--- a/internal/rcm/api_test.go
+++ b/internal/rcm/api_test.go
@@ -3,6 +3,7 @@ package rcm_test
 import (
 	"bytes"
 	"encoding/json"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"regexp"
@@ -44,7 +45,7 @@ func TestBasicRcmAPI(t *testing.T) {
 		{"GET", "/v1/compose", ``, "", http.StatusMethodNotAllowed, ``},
 		{"POST", "/v1/compose", `{"status":"RUNNING"}`, "application/json", http.StatusBadRequest, ``},
 		{"POST", "/v1/compose", `{"status":"RUNNING"}`, "text/plain", http.StatusBadRequest, ``},
-		{"POST", "/v1/compose", `{"distribution": "fedora-30", "image_types": ["qcow2"], "architectures":["x86_64"], "repositories": []}`, "application/json", http.StatusBadRequest, ""},
+		{"POST", "/v1/compose", `{"image_builds":[]}`, "application/json", http.StatusBadRequest, ""},
 		{"POST", "/v1/compose/111-222-333", `{"status":"RUNNING"}`, "application/json", http.StatusMethodNotAllowed, ``},
 		{"GET", "/v1/compose/7802c476-9cd1-41b7-ba81-43c1906bce73", `{"status":"RUNNING"}`, "application/json", http.StatusBadRequest, `{"error_reason":"Compose UUID does not exist"}`},
 	}
@@ -73,8 +74,7 @@ func TestBasicRcmAPI(t *testing.T) {
 	}
 }
 
-func TestSubmitCompose(t *testing.T) {
-	// Test the most basic use case: Submit a new job and get its status.
+func TestSubmit(t *testing.T) {
 	registry, err := distro_mock.NewDefaultRegistry()
 	if err != nil {
 		t.Fatal(err)
@@ -84,27 +84,101 @@ func TestSubmitCompose(t *testing.T) {
 	var submit_reply struct {
 		UUID uuid.UUID `json:"compose_id"`
 	}
-	var status_reply struct {
-		Status      string `json:"status,omitempty"`
-		ErrorReason string `json:"error_reason,omitempty"`
-	}
 
 	var cases = []struct {
-		Method      string
-		Path        string
-		Body        string
-		ContentType string
+		Method         string
+		Path           string
+		Body           string
+		ContentType    string
+		ExpectedStatus int
 	}{
 		{
 			"POST",
 			"/v1/compose",
-			`{"distribution": "fedora-30", 
-					"image_types": ["qcow2"], 
-					"architectures":["x86_64"], 
-					"repositories": [{
-						"url": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
-					}]}`,
+			`{
+				"image_builds":
+				[
+					{
+						"distribution": "fedora-30",
+						"architecture": "x86_64",
+						"image_type": "qcow2",
+						"repositories":
+						[
+							{
+								"baseurl": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
+							}
+						]
+					}
+				]
+			}`,
+			"application/json",
+			http.StatusOK,
+		},
+		{
+			"POST",
+			"/v1/compose",
+			`{
+				"image_builds":
+				[
+					{
+						"distribution": "invalid",
+						"architecture": "x86_64",
+						"image_type": "qcow2",
+						"repositories":
+						[
+							{
+								"baseurl": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
+							}
+						]
+					}
+				]
+			}`,
+			"application/json",
+			http.StatusBadRequest,
+		},
+		{
+			"POST",
+			"/v1/compose",
+			`{
+				"image_builds":
+				[
+					{
+						"distribution": "fedora-30",
+						"architecture": "invalid",
+						"image_type": "qcow2",
+						"repositories":
+						[
+							{
+								"baseurl": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
+							}
+						]
+					}
+				]
+			}`,
+			"application/json",
+			http.StatusBadRequest,
+		},
+		{
+			"POST",
+			"/v1/compose",
+			`{
+				"image_builds":
+				[
+					{
+						"distribution": "fedora-30",
+						"architecture": "x86_64",
+						"image_type": "invalid",
+						"repositories":
+						[
+							{
+								"baseurl": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
+							}
+						]
+					}
+				]
+			}`,
 			"application/json",
+			http.StatusBadRequest,
 		},
 	}
 
@@ -112,26 +186,96 @@ func TestSubmitCompose(t *testing.T) {
 		// Submit job
 		t.Logf("RCM API submit compose, case %d\n", n)
 		resp := internalRequest(api, c.Method, c.Path, c.Body, c.ContentType)
-		if resp.StatusCode != http.StatusOK {
-			t.Fatal("Failed to call /v1/compose, HTTP status code:", resp.StatusCode)
+		if resp.StatusCode != c.ExpectedStatus {
+			errReason, _ := ioutil.ReadAll(resp.Body)
+			t.Fatal("Failed to call /v1/compose, HTTP status code: ", resp.StatusCode, "Error message: ", string(errReason))
 		}
-		decoder := json.NewDecoder(resp.Body)
-		decoder.DisallowUnknownFields()
-		err := decoder.Decode(&submit_reply)
-		if err != nil {
-			t.Fatal("Failed to decode response to /v1/compose:", err)
+		if resp.StatusCode == http.StatusOK {
+			decoder := json.NewDecoder(resp.Body)
+			decoder.DisallowUnknownFields()
+			err := decoder.Decode(&submit_reply)
+			if err != nil {
+				t.Fatal("Failed to decode response to /v1/compose:", err)
+			}
 		}
+	}
+}
+
+func TestStatus(t *testing.T) {
+	// Test the most basic use case: Submit a new job and get its status.
+	registry, err := distro_mock.NewDefaultRegistry()
+	if err != nil {
+		t.Fatal(err)
+	}
+	api := rcm.New(nil, store.New(nil), rpmmd_mock.NewRPMMDMock(rpmmd_mock.BaseFixture()), registry)
+
+	var submit_reply struct {
+		UUID uuid.UUID `json:"compose_id"`
+	}
+	var status_reply struct {
+		Status      string `json:"status,omitempty"`
+		ErrorReason string `json:"error_reason,omitempty"`
+	}
+	// Submit a job
+	resp := internalRequest(api,
+		"POST",
+		"/v1/compose",
+		`{
+				"image_builds":
+				[
+					{
+						"distribution": "fedora-30",
+						"architecture": "x86_64",
+						"image_type": "qcow2",
+						"repositories":
+						[
+							{
+								"baseurl": "http://download.fedoraproject.org/pub/fedora/linux/releases/30/Everything/x86_64/os/"
+							}
+						]
+					}
+				]
+			}`,
+		"application/json")
+	if resp.StatusCode != http.StatusOK {
+		errReason, _ := ioutil.ReadAll(resp.Body)
+		t.Fatal("Failed to call /v1/compose, HTTP status code: ", resp.StatusCode, "Error message: ", string(errReason))
+	}
+	decoder := json.NewDecoder(resp.Body)
+	decoder.DisallowUnknownFields()
+	err = decoder.Decode(&submit_reply)
+	if err != nil {
+		t.Fatal("Failed to decode response to /v1/compose:", err)
+	}
+
+	var cases = []struct {
+		Method         string
+		Path           string
+		Body           string
+		ContentType    string
+		ExpectedStatus string
+	}{
+		{
+			"GET",
+			"/v1/compose/" + submit_reply.UUID.String(),
+			``,
+			"application/json",
+			"WAITING",
+		},
+	}
+
+	for n, c := range cases {
 		// Get the status
-		t.Log("RCM API get status")
-		resp = internalRequest(api, "GET", "/v1/compose/"+submit_reply.UUID.String(), "", "")
+		t.Logf("RCM API get compose status, case %d\n", n)
+		resp = internalRequest(api, c.Method, c.Path, c.Body, c.ContentType)
 		decoder = json.NewDecoder(resp.Body)
 		decoder.DisallowUnknownFields()
 		err = decoder.Decode(&status_reply)
 		if err != nil {
 			t.Fatal("Failed to decode response to /v1/compose/UUID:", err)
 		}
-		if status_reply.ErrorReason != "" {
-			t.Error("Failed to get compose status, reason:", status_reply.ErrorReason)
+		if status_reply.Status != c.ExpectedStatus {
+			t.Error("Failed to get compose status:", status_reply.Status)
 		}
 	}
 }