Repository: osbuild/osbuild-composer
Pull request #2051 (2021-11-22 13:38:26)

osbuild2: update cloud-init stage with new options
Related to: https://github.com/osbuild/osbuild/pull/866/


This pull request includes:

- [ ] adequate testing for the new functionality or fixed issue
- [ ] adequate documentation informing people about the change such as
  - [ ] submit a PR for the [guides](https://github.com/osbuild/guides) repository if this PR changed any behavior described there: https://www.osbuild.org/guides/

<!--
Thanks for proposing a change to osbuild-composer!

Please don't remove the above check list. These are things that each pull
request must have before it is merged. It helps maintainers to not forget
anything.

If the reason for ticking any of the boxes is ambiguous, please add a short
note explaining why.

In addition, if this pull request fixes a downstream issue, please refer to
test/README.md and add these additional items:

- [ ] 1st commit of any `rhbz#` related PR contains bug reproducer; CI reports FAIL or
- [ ] PR contains automated tests for new functionality and
- [ ] QE has approved reproducer/new tests and
- [ ] Subsequent commits provide bug fixes without modifying the reproducer; CI reports PASS and
- [ ] QE approves this PR; RHBZ status is set to `MODIFIED + Verified=Tested`
-->


msehnout: > We had a discussion recently about doing the option validation in the `NewXStage()` constructor instead of the `MarshalJSON()` method. #1785

Yeah, I read that one. Actually that's why I created all those `validate()` functions, so that I can call it in the `NewXStage`. Just forgot to actually call it :facepalm: 
msehnout: @achilleas-k Please have a look again. I wasn't sure whether to leave the `validate()` calls in the MarshalJSON functions or not. Right now the constructor as well as the marshalers call the functions, but it is a lot of boilerplate code. If we are ok with removing the validation from marshalers, we can remove a lot of lines and call the validation recursively (I didn't want to call `validate()` on internal variables in all the structures, because MarshalJSON calls MarshalJSON for every internal variable and in turn it calls `validate()`)
achilleas-k: Looks good.  Having a nested/recursive `validate()` call that starts from the `NewStage()` is much nicer I think.  Let's clean the custom marshallers, which should also remove the need for the type aliases.  The whole thing should look much cleaner then.
msehnout: I agree. The PR is updated and simplified now :-) 

Patch:
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index dc26d66e2..8d8b9e5d9 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -36,6 +36,7 @@ init:
     - shell
   script:
     - schutzbot/update_github_status.sh start
+  interruptible: true
 
 RPM:
   stage: rpmbuild
@@ -44,6 +45,7 @@ RPM:
     - if: '$CI_PIPELINE_SOURCE != "schedule"'
   script:
     - sh "schutzbot/mockbuild.sh"
+  interruptible: true
   after_script:
     - schutzbot/update_github_status.sh update
     - schutzbot/save_journal.sh
@@ -75,6 +77,7 @@ Container:
     - if: '$CI_PIPELINE_SOURCE != "schedule"'
   script:
     - sh "schutzbot/containerbuild.sh"
+  interruptible: true
   parallel:
     matrix:
       - RUNNER:
@@ -88,6 +91,7 @@ Prepare-rhel-internal:
     - if: '$CI_PIPELINE_SOURCE == "schedule" && $RUNNER =~ /[\S]+rhel-8.6-[^ga][\S]+/ && $NIGHTLY == "true" && $RHEL_MAJOR == "8"'
   script:
     - schutzbot/prepare-rhel-internal.sh
+  interruptible: true
   artifacts:
     paths:
       - rhel-${RHEL_MAJOR}.json
@@ -113,6 +117,7 @@ Base:
   script:
     - schutzbot/deploy.sh
     - /usr/libexec/tests/osbuild-composer/base_tests.sh
+  interruptible: true
   parallel:
     matrix:
       - RUNNER:
@@ -168,6 +173,7 @@ Regression:
   script:
     - schutzbot/deploy.sh
     - /usr/libexec/tests/osbuild-composer/regression.sh
+  interruptible: true
   parallel:
     matrix:
       - RUNNER:
@@ -200,6 +206,7 @@ OSTree:
   script:
     - schutzbot/deploy.sh
     - /usr/libexec/tests/osbuild-composer/ostree.sh
+  interruptible: true
   parallel:
     matrix:
       - RUNNER:
@@ -345,6 +352,7 @@ libvirt:
   script:
     - schutzbot/deploy.sh
     - /usr/libexec/tests/osbuild-composer/libvirt.sh
+  interruptible: true
   parallel:
     matrix:
       - RUNNER:
@@ -364,6 +372,7 @@ RHEL 9 on 8:
   script:
     - schutzbot/deploy.sh
     - /usr/libexec/tests/osbuild-composer/koji.sh
+  interruptible: true
   variables:
     RUNNER: aws/rhel-8.4-ga-x86_64
     INTERNAL_NETWORK: "true"
@@ -398,6 +407,7 @@ Installer:
   script:
     - schutzbot/deploy.sh
     - /usr/libexec/tests/osbuild-composer/installers.sh
+  interruptible: true
   parallel:
     matrix:
       - RUNNER:
diff --git a/internal/kojiapi/server.go b/internal/kojiapi/server.go
index b26ccd8d8..d7fb582b4 100644
--- a/internal/kojiapi/server.go
+++ b/internal/kojiapi/server.go
@@ -400,7 +400,7 @@ func (h *apiHandlers) GetComposeIdLogs(ctx echo.Context, idstr string) error {
 		if _, _, err := h.getBuildJob(deps[i]); err != nil {
 			panic(err)
 		}
-		var buildResult worker.OSBuildJobResult
+		var buildResult worker.OSBuildKojiJobResult
 		_, _, err = h.server.workers.JobStatus(deps[i], &buildResult)
 		if err != nil {
 			// This is a programming error.
diff --git a/internal/osbuild2/cloud_init_stage.go b/internal/osbuild2/cloud_init_stage.go
index c12e4a015..5ac3be086 100644
--- a/internal/osbuild2/cloud_init_stage.go
+++ b/internal/osbuild2/cloud_init_stage.go
@@ -1,7 +1,6 @@
 package osbuild2
 
 import (
-	"encoding/json"
 	"fmt"
 )
 
@@ -13,6 +12,9 @@ type CloudInitStageOptions struct {
 func (CloudInitStageOptions) isStageOptions() {}
 
 func NewCloudInitStage(options *CloudInitStageOptions) *Stage {
+	if err := options.Config.validate(); err != nil {
+		panic(err)
+	}
 	return &Stage{
 		Type:    "org.osbuild.cloud-init",
 		Options: options,
@@ -21,18 +23,11 @@ func NewCloudInitStage(options *CloudInitStageOptions) *Stage {
 
 // Represents a cloud-init configuration file
 type CloudInitConfigFile struct {
-	SystemInfo *CloudInitConfigSystemInfo `json:"system_info,omitempty"`
-}
-
-// Unexported alias for use in CloudInitConfigFile's MarshalJSON() to prevent recursion
-type cloudInitConfigFile CloudInitConfigFile
-
-func (c CloudInitConfigFile) MarshalJSON() ([]byte, error) {
-	if c.SystemInfo == nil {
-		return nil, fmt.Errorf("at least one cloud-init configuration option must be specified")
-	}
-	configFile := cloudInitConfigFile(c)
-	return json.Marshal(configFile)
+	SystemInfo     *CloudInitConfigSystemInfo `json:"system_info,omitempty"`
+	Reporting      *CloudInitConfigReporting  `json:"reporting,omitempty"`
+	Datasource     *CloudInitConfigDatasource `json:"datasource,omitempty"`
+	DatasourceList []string                   `json:"datasource_list,omitempty"`
+	Output         *CloudInitConfigOutput     `json:"output,omitempty"`
 }
 
 // Represents the 'system_info' configuration section
@@ -40,15 +35,31 @@ type CloudInitConfigSystemInfo struct {
 	DefaultUser *CloudInitConfigDefaultUser `json:"default_user,omitempty"`
 }
 
-// Unexported alias for use in CloudInitConfigSystemInfo's MarshalJSON() to prevent recursion
-type cloudInitConfigSystemInfo CloudInitConfigSystemInfo
+// Represents the 'reporting' configuration section
+type CloudInitConfigReporting struct {
+	Logging   *CloudInitConfigReportingHandlers `json:"logging,omitempty"`
+	Telemetry *CloudInitConfigReportingHandlers `json:"telemetry,omitempty"`
+}
 
-func (si CloudInitConfigSystemInfo) MarshalJSON() ([]byte, error) {
-	if si.DefaultUser == nil {
-		return nil, fmt.Errorf("at least one configuration option must be specified for 'system_info' section")
-	}
-	systemInfo := cloudInitConfigSystemInfo(si)
-	return json.Marshal(systemInfo)
+type CloudInitConfigReportingHandlers struct {
+	Type string `json:"type"`
+}
+
+// Represents the 'datasource' configuration section
+type CloudInitConfigDatasource struct {
+	Azure *CloudInitConfigDatasourceAzure `json:"Azure,omitempty"`
+}
+
+type CloudInitConfigDatasourceAzure struct {
+	ApplyNetworkConfig bool `json:"apply_network_config"`
+}
+
+// Represents the 'output' configuration section
+type CloudInitConfigOutput struct {
+	Init   *string `json:"init,omitempty"`
+	Config *string `json:"config,omitempty"`
+	Final  *string `json:"final,omitempty"`
+	All    *string `json:"all,omitempty"`
 }
 
 // Configuration of the 'default' user created by cloud-init.
@@ -56,13 +67,92 @@ type CloudInitConfigDefaultUser struct {
 	Name string `json:"name,omitempty"`
 }
 
-// Unexported alias for use in CloudInitConfigDefaultUser's MarshalJSON() to prevent recursion
-type cloudInitConfigDefaultUser CloudInitConfigDefaultUser
+func (c CloudInitConfigFile) validate() error {
+	if c.SystemInfo == nil && c.Reporting == nil && c.Datasource == nil && len(c.DatasourceList) == 0 && c.Output == nil {
+		return fmt.Errorf("at least one cloud-init configuration option must be specified")
+	}
+	if c.SystemInfo != nil {
+		if err := c.SystemInfo.validate(); err != nil {
+			return err
+		}
+	}
+	if c.Reporting != nil {
+		if err := c.Reporting.validate(); err != nil {
+			return err
+		}
+	}
+	if c.Datasource != nil {
+		if err := c.Datasource.validate(); err != nil {
+			return err
+		}
+	}
+	if len(c.DatasourceList) > 0 {
+		for _, d := range c.DatasourceList {
+			if d != "Azure" {
+				return fmt.Errorf("only 'Azure' is allowed as an item in the datasource_list")
+			}
+		}
+	}
+	if c.Output != nil {
+		if err := c.Output.validate(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (si CloudInitConfigSystemInfo) validate() error {
+	if si.DefaultUser == nil {
+		return fmt.Errorf("at least one configuration option must be specified for 'system_info' section")
+	} else {
+		return si.DefaultUser.validate()
+	}
+}
+
+func (r CloudInitConfigReporting) validate() error {
+	if r.Logging == nil && r.Telemetry == nil {
+		return fmt.Errorf("at least one configuration option must be specified for 'reporting' section")
+	}
+	if r.Logging != nil {
+		if err := r.Logging.validate(); err != nil {
+			return err
+		}
+	}
+	if r.Telemetry != nil {
+		if err := r.Telemetry.validate(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (r CloudInitConfigReportingHandlers) validate() error {
+	allowed_values := []string{"log", "print", "webhook", "hyperv"}
+	for _, v := range allowed_values {
+		if v == r.Type {
+			return nil
+		}
+	}
+	return fmt.Errorf("reporting parameters must be one of 'log', 'print', 'webhook', 'hyperv'")
+}
+
+func (d CloudInitConfigDatasource) validate() error {
+	if d.Azure == nil {
+		return fmt.Errorf("at least one configuration option must be specified for 'datasource' section")
+	}
+	return nil
+}
+
+func (o CloudInitConfigOutput) validate() error {
+	if o.Init == nil && o.Config == nil && o.Final == nil && o.All == nil {
+		return fmt.Errorf("at least one configuration option must be specified for 'output' section")
+	}
+	return nil
+}
 
-func (du CloudInitConfigDefaultUser) MarshalJSON() ([]byte, error) {
+func (du CloudInitConfigDefaultUser) validate() error {
 	if du.Name == "" {
-		return nil, fmt.Errorf("at least one configuration option must be specified for 'default_user' section")
+		return fmt.Errorf("at least one configuration option must be specified for 'default_user' section")
 	}
-	defaultUser := cloudInitConfigDefaultUser(du)
-	return json.Marshal(defaultUser)
+	return nil
 }


Test patch:
diff --git a/internal/osbuild2/cloud_init_stage_test.go b/internal/osbuild2/cloud_init_stage_test.go
index 93d2e438f..6eada673b 100644
--- a/internal/osbuild2/cloud_init_stage_test.go
+++ b/internal/osbuild2/cloud_init_stage_test.go
@@ -2,21 +2,41 @@ package osbuild2
 
 import (
 	"encoding/json"
+	"reflect"
 	"testing"
 
+	"github.com/osbuild/osbuild-composer/internal/common"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestNewCloudInitStage(t *testing.T) {
 	expectedStage := &Stage{
-		Type:    "org.osbuild.cloud-init",
-		Options: &CloudInitStageOptions{},
+		Type: "org.osbuild.cloud-init",
+		Options: &CloudInitStageOptions{
+			Filename: "aaa",
+			Config: CloudInitConfigFile{
+				SystemInfo: &CloudInitConfigSystemInfo{
+					DefaultUser: &CloudInitConfigDefaultUser{
+						Name: "foo",
+					},
+				},
+			},
+		},
 	}
-	actualStage := NewCloudInitStage(&CloudInitStageOptions{})
+	actualStage := NewCloudInitStage(&CloudInitStageOptions{
+		Filename: "aaa",
+		Config: CloudInitConfigFile{
+			SystemInfo: &CloudInitConfigSystemInfo{
+				DefaultUser: &CloudInitConfigDefaultUser{
+					Name: "foo",
+				},
+			},
+		},
+	})
 	assert.Equal(t, expectedStage, actualStage)
 }
 
-func TestCloudInitStage_MarshalJSON_Invalid(t *testing.T) {
+func TestCloudInitStage_NewStage_Invalid(t *testing.T) {
 	tests := []struct {
 		name    string
 		options CloudInitStageOptions
@@ -54,9 +74,142 @@ func TestCloudInitStage_MarshalJSON_Invalid(t *testing.T) {
 		},
 	}
 	for idx, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Panics(t, func() { NewCloudInitStage(&tt.options) }, "NewCloudInitStage didn't panic, but it should [idx: %d]", idx)
+		})
+	}
+}
+
+func TestCloudInitStage_MarshalJSON(t *testing.T) {
+	tests := []struct {
+		name    string
+		options CloudInitStageOptions
+		json    string
+	}{
+		{
+			name: "simple-cloud-init-config-with-system-info",
+			options: CloudInitStageOptions{
+				Config: CloudInitConfigFile{
+					SystemInfo: &CloudInitConfigSystemInfo{
+						DefaultUser: &CloudInitConfigDefaultUser{
+							Name: "foo",
+						},
+					},
+				},
+			},
+			json: `{"filename":"","config":{"system_info":{"default_user":{"name":"foo"}}}}`,
+		},
+	}
+	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			gotBytes, err := json.Marshal(tt.options)
-			assert.NotNilf(t, err, "json.Marshal() didn't return an error, but: %s [idx: %d]", string(gotBytes), idx)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.json, string(gotBytes))
+		})
+	}
+}
+
+func TestCloudInitStage_UnmarshalJSON(t *testing.T) {
+	tests := []struct {
+		name    string
+		options CloudInitStageOptions
+		json    string
+	}{
+		{
+			name: "simple-cloud-init-config-with-system-info",
+			options: CloudInitStageOptions{
+				Config: CloudInitConfigFile{
+					SystemInfo: &CloudInitConfigSystemInfo{
+						DefaultUser: &CloudInitConfigDefaultUser{
+							Name: "foo",
+						},
+					},
+				},
+			},
+			json: `{"filename":"","config":{"system_info":{"default_user":{"name":"foo"}}}}`,
+		},
+		{
+			name: "osbuild-test-suite-1",
+			options: CloudInitStageOptions{
+				Filename: "10-azure-kfp.cfg",
+				Config: CloudInitConfigFile{
+					Reporting: &CloudInitConfigReporting{
+						Logging: &CloudInitConfigReportingHandlers{
+							Type: "log",
+						},
+						Telemetry: &CloudInitConfigReportingHandlers{
+							Type: "hyperv",
+						},
+					},
+				},
+			},
+			json: `{
+				"filename": "10-azure-kfp.cfg",
+				"config": {
+				  "reporting": {
+					"logging": {
+					  "type": "log"
+					},
+					"telemetry": {
+					  "type": "hyperv"
+					}
+				  }
+				}
+			  }`,
+		},
+		{
+			name: "osbuild-test-suite-2",
+			options: CloudInitStageOptions{
+				Filename: "91-azure_datasource.cfg",
+				Config: CloudInitConfigFile{
+					DatasourceList: []string{"Azure"},
+					Datasource: &CloudInitConfigDatasource{
+						Azure: &CloudInitConfigDatasourceAzure{
+							ApplyNetworkConfig: false,
+						},
+					},
+				},
+			},
+			json: `{
+				"filename": "91-azure_datasource.cfg",
+				"config": {
+				  "datasource_list": [
+					"Azure"
+				  ],
+				  "datasource": {
+					"Azure": {
+					  "apply_network_config": false
+					}
+				  }
+				}
+			  }`,
+		},
+		{
+			name: "osbuild-test-suite-3",
+			options: CloudInitStageOptions{
+				Filename: "06_logging_override.cfg",
+				Config: CloudInitConfigFile{
+					Output: &CloudInitConfigOutput{
+						All: common.StringToPtr(">> /var/log/cloud-init-all.log"),
+					},
+				},
+			},
+			json: `{
+				"filename": "06_logging_override.cfg",
+				"config": {
+				  "output": {
+					"all": ">> /var/log/cloud-init-all.log"
+				  }
+				}
+			  }`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var gotOptions CloudInitStageOptions
+			err := json.Unmarshal([]byte(tt.json), &gotOptions)
+			assert.NoError(t, err)
+			assert.True(t, reflect.DeepEqual(tt.options, gotOptions))
 		})
 	}
 }
diff --git a/test/README.md b/test/README.md
index 6a374a3c4..31c92d5a5 100644
--- a/test/README.md
+++ b/test/README.md
@@ -74,6 +74,8 @@ the same architecture, as the one intended for the generated test
 cases. In other words, you need to generate e.g test cases for `aarch64`
 images on an `aarch64` host.
 
+These tests are executed from the `osbuild` repository in GitHub CI.
+
 **Important Note:** `image-info` by default won't be able to read SELinux
 labels used in the image, which are unknown to the host's policy. If you are
 generating the image test case using
@@ -395,16 +397,17 @@ is is missing lots of details!
 - [@jrusz](https://github.com/jrusz) - testing across the board; backup for @atodorov;
   osbuild, osbuild-composer, weldr-client, composer-cli, image-builder, image-builder-frontend,
   bug testing & verification, anything else
+- [@jabia99](https://github.com/jabia99) - focus on image-builder and image-builder-frontend;
+  anything else; backup for @jrusz
 - [@henrywang](https://github.com/henrywang) - everything related to testing RHEL for Edge images
 - [@yih-redhat](https://github.com/yih-redhat) - everything related to testing RHEL for Edge images;
   backup for @henrywang
-- [@yuxisun1217](https://github.com/yuxisun1217) - QE for testing image-builder and image-builder-frontend
 
 
 ## Internal build testing in Gitlab CI
 
 In Gitlab CI we're using rules to define which stages and jobs are ran when. For running the test
-suite on internal rhel builds we're using schedules that run the test suite on latest nightly using
+suite on internal RHEL builds we're using schedules that run the test suite on latest nightly using
 the rpms that are shipped with it.
 
 We're using a link to latest nightly build. It's possible to manually override it with `COMPOSE_URL`