code review

- moved getConfigRef to utils file to be reused
- added test for not valid configuration
- renamed DescribeConfiguration into Configuration

Author: Daniele Marostica

internal/cmd/project/apply.go

@@ -18,8 +18,6 @@ package project
 import (
 	"context"
 	"fmt"
-	"io"
-	"net/url"
 
 	"github.com/mia-platform/miactl/internal/client"
 	"github.com/mia-platform/miactl/internal/clioptions"
@@ -46,12 +44,6 @@ type applyProjectOptions struct {
 	FilePath     string
 }
 
-type ApplyProjectConfigurationRequest struct {
-	Title        string `json:"title" yaml:"title"`
-	PreviousSave string `json:"previousSave,omitempty" yaml:"previousSave,omitempty"`
-	*configuration.DescribeConfiguration
-}
-
 // ApplyCmd returns a cobra command for applying a project configuration
 func ApplyCmd(options *clioptions.CLIOptions) *cobra.Command {
 	cmd := &cobra.Command{
@@ -71,7 +63,7 @@ func ApplyCmd(options *clioptions.CLIOptions) *cobra.Command {
 				FilePath:     options.InputFilePath,
 			}
 
-			return handleApplyProjectConfigurationCmd(cmd.Context(), client, cmdOptions, cmd.OutOrStdout())
+			return handleApplyProjectConfigurationCmd(cmd.Context(), client, cmdOptions)
 		},
 	}
 
@@ -87,7 +79,7 @@ func ApplyCmd(options *clioptions.CLIOptions) *cobra.Command {
 	return cmd
 }
 
-func handleApplyProjectConfigurationCmd(ctx context.Context, client *client.APIClient, options applyProjectOptions, writer io.Writer) error {
+func handleApplyProjectConfigurationCmd(ctx context.Context, client *client.APIClient, options applyProjectOptions) error {
 	err := validateApplyProjectOptions(options)
 	if err != nil {
 		return err
@@ -98,7 +90,7 @@ func handleApplyProjectConfigurationCmd(ctx context.Context, client *client.APIC
 		return fmt.Errorf("failed to apply project configuration: %w", err)
 	}
 
-	fmt.Fprintln(writer, "Project configuration applied successfully")
+	fmt.Println("Project configuration applied successfully")
 	return nil
 }
 
@@ -119,7 +111,7 @@ func validateApplyProjectOptions(options applyProjectOptions) error {
 }
 
 func applyConfiguration(ctx context.Context, client *client.APIClient, options applyProjectOptions) error {
-	ref, err := getRevisionRef(options.RevisionName)
+	ref, err := configuration.GetEncodedRevisionRef(options.RevisionName)
 	if err != nil {
 		return err
 	}
@@ -135,10 +127,10 @@ func applyConfiguration(ctx context.Context, client *client.APIClient, options a
 	}
 
 	previousSnapshotID := structuredConfig.Config["commitId"].(string)
-	applyConfig := ApplyProjectConfigurationRequest{
-		DescribeConfiguration: structuredConfig,
-		Title:                 "[miactl] Applied project configuration",
-		PreviousSave:          previousSnapshotID,
+	applyConfig := configuration.ApplyRequest{
+		Configuration: structuredConfig,
+		Title:         "[miactl] Applied project configuration",
+		PreviousSave:  previousSnapshotID,
 	}
 
 	body, err := resources.EncodeResourceToJSON(applyConfig)
@@ -161,12 +153,3 @@ func applyConfiguration(ctx context.Context, client *client.APIClient, options a
 
 	return nil
 }
-
-func getRevisionRef(revisionName string) (string, error) {
-	if len(revisionName) == 0 {
-		return "", fmt.Errorf("missing revision name, please provide a revision name")
-	}
-
-	encodedRevisionName := url.PathEscape(revisionName)
-	return fmt.Sprintf("revisions/%s", encodedRevisionName), nil
-}

internal/cmd/project/apply_test.go

@@ -16,7 +16,6 @@
 package project
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"net/http"
@@ -44,7 +43,6 @@ func TestApplyProjectCmd(t *testing.T) {
 		expectError      bool
 		expectedErrorMsg string
 		testServer       *httptest.Server
-		expectedRequest  string
 	}{
 		"error missing project id": {
 			options:          applyProjectOptions{},
@@ -87,6 +85,18 @@ func TestApplyProjectCmd(t *testing.T) {
 				return false
 			}),
 		},
+		"error invalid configuration": {
+			options: applyProjectOptions{
+				ProjectID:    "test-project",
+				RevisionName: "test-revision",
+				FilePath:     "testdata/not-valid-configuration.json",
+			},
+			expectError:      true,
+			expectedErrorMsg: "provided configuration is not valid",
+			testServer: applyTestServer(t, func(_ http.ResponseWriter, _ *http.Request) bool {
+				return false
+			}),
+		},
 		"apply base project (JSON)": {
 			options: applyProjectOptions{
 				ProjectID:    "test-project",
@@ -237,15 +247,14 @@ func TestApplyProjectCmd(t *testing.T) {
 			})
 			require.NoError(t, err)
 
-			var writer bytes.Buffer
-			err = handleApplyProjectConfigurationCmd(ctx, client, testCase.options, &writer)
+			err = handleApplyProjectConfigurationCmd(ctx, client, testCase.options)
 
 			if testCase.expectError {
 				assert.Error(t, err)
 				assert.Contains(t, err.Error(), testCase.expectedErrorMsg)
 			} else {
 				assert.NoError(t, err)
-				assert.Contains(t, writer.String(), "Project configuration applied successfully")
+				// assert.Contains(t, writer.String(), "Project configuration applied successfully")
 			}
 		})
 	}

internal/cmd/project/describe.go

@@ -19,7 +19,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"net/url"
 
 	"github.com/mia-platform/miactl/internal/client"
 	"github.com/mia-platform/miactl/internal/clioptions"
@@ -79,7 +78,7 @@ func describeProject(ctx context.Context, client *client.APIClient, options desc
 		return fmt.Errorf("missing project name, please provide a project name as argument")
 	}
 
-	ref, err := getConfigRef(options.RevisionName, options.VersionName)
+	ref, err := configuration.GetEncodedRef(options.RevisionName, options.VersionName)
 	if err != nil {
 		return err
 	}
@@ -114,20 +113,3 @@ func describeProject(ctx context.Context, client *client.APIClient, options desc
 	fmt.Fprintln(writer, string(bytes))
 	return nil
 }
-
-func getConfigRef(revisionName, versionName string) (string, error) {
-	if len(revisionName) > 0 && len(versionName) > 0 {
-		return "", fmt.Errorf("both revision and version specified, please provide only one")
-	}
-
-	if len(revisionName) > 0 {
-		encodedRevisionName := url.PathEscape(revisionName)
-		return fmt.Sprintf("revisions/%s", encodedRevisionName), nil
-	}
-	if len(versionName) > 0 {
-		encodedVersionName := url.PathEscape(versionName)
-		return fmt.Sprintf("versions/%s", encodedVersionName), nil
-	}
-
-	return "", fmt.Errorf("missing revision/version name, please provide one as argument")
-}

internal/cmd/project/testdata/not-valid-configuration.json

@@ -0,0 +1,7 @@
+{
+    "field1": "value1",
+    "field2": {
+        "subfield1": "subvalue1",
+        "subfield2": "subvalue2"
+    }
+}

internal/resources/configuration/applyrequest.go

@@ -0,0 +1,22 @@
+// Copyright Mia srl
+// SPDX-License-Identifier: Apache-2.0
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package configuration
+
+type ApplyRequest struct {
+	Title        string `json:"title" yaml:"title"`
+	PreviousSave string `json:"previousSave,omitempty" yaml:"previousSave,omitempty"`
+	*Configuration
+}

internal/resources/configuration/describe.go renamed to internal/resources/configuration/configuration.go

@@ -17,34 +17,39 @@ package configuration
 
 import "fmt"
 
-type DescribeConfiguration struct {
+const (
+	ErrNotValidConfiguration = "provided configuration is not valid"
+)
+
+type Configuration struct {
 	Config                     map[string]any `json:"config" yaml:"config"`
 	FastDataConfig             map[string]any `json:"fastDataConfig,omitempty" yaml:"fastDataConfig,omitempty"`
 	MicrofrontendPluginsConfig map[string]any `json:"microfrontendPluginsConfig,omitempty" yaml:"microfrontendPluginsConfig,omitempty"`
 	ExtensionsConfig           map[string]any `json:"extensionsConfig,omitempty" yaml:"extensionsConfig,omitempty"`
 }
 
-func BuildDescribeConfiguration(config map[string]any) (*DescribeConfiguration, error) {
-	return describeConfigurationAdapter(config)
+// BuildDescribeConfiguration builds a DescribeConfiguration from a configuration
+func BuildDescribeConfiguration(rawConfig map[string]any) (*Configuration, error) {
+	return describeConfigurationAdapter(rawConfig)
 }
 
-func BuildDescribeFromFlatConfiguration(config map[string]any) (*DescribeConfiguration, error) {
-	return flatConfigurationAdapter(config)
+func BuildDescribeFromFlatConfiguration(rawConfig map[string]any) (*Configuration, error) {
+	return flatConfigurationAdapter(rawConfig)
 }
 
 // describeConfigurationAdapter adapts an already describe configuration map to the expected DescribeConfiguration type.
-// This is used to adapt output from the describe command (e.g., when reading the describe output from a file)
-func describeConfigurationAdapter(config map[string]any) (*DescribeConfiguration, error) {
-	applyConfig := &DescribeConfiguration{}
+// This is used to adapt output from the describe command (e.g., when reading the describe output from a file) to a valid DescribeConfiguration structure.
+func describeConfigurationAdapter(config map[string]any) (*Configuration, error) {
+	applyConfig := &Configuration{}
 
 	configSection, hasConfigKey := config["config"]
 	if !hasConfigKey {
-		return nil, fmt.Errorf("not a structured configuration: 'config' key not found")
+		return nil, fmt.Errorf("%s: %s", ErrNotValidConfiguration, "'config' key not found")
 	}
 
 	configSectionMap, ok := configSection.(map[string]any)
 	if !ok {
-		return nil, fmt.Errorf("invalid structured configuration provided: 'config' key is not a valid map[string]any")
+		return nil, fmt.Errorf("%s: %s", ErrNotValidConfiguration, "'config' key is not a valid map[string]any")
 	}
 
 	baseConfig := make(map[string]any)
@@ -69,9 +74,9 @@ func describeConfigurationAdapter(config map[string]any) (*DescribeConfiguration
 }
 
 // AdaptFlatConfiguration adapts a flat configuration map to the structured format.
-// This is used to map the GET /configuration response to the expected structure.
-func flatConfigurationAdapter(config map[string]any) (*DescribeConfiguration, error) {
-	applyConfig := &DescribeConfiguration{}
+// This is used to map the GET /configuration response into the DescribeConfiguration structure.
+func flatConfigurationAdapter(config map[string]any) (*Configuration, error) {
+	applyConfig := &Configuration{}
 
 	baseConfig := make(map[string]any)
 	for key, value := range config {

internal/resources/configuration/utils.go

@@ -0,0 +1,55 @@
+// Copyright Mia srl
+// SPDX-License-Identifier: Apache-2.0
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package configuration
+
+import (
+	"fmt"
+	"net/url"
+)
+
+func GetEncodedRef(revisionName, versionName string) (string, error) {
+	if len(revisionName) > 0 && len(versionName) > 0 {
+		return "", fmt.Errorf("both revision and version specified, please provide only one")
+	}
+
+	if len(revisionName) > 0 {
+		return GetEncodedRevisionRef(revisionName)
+	}
+
+	if len(versionName) > 0 {
+		return GetEncodedVersionRef(versionName)
+	}
+
+	return "", fmt.Errorf("missing revision/version name, please provide one as argument")
+}
+
+func GetEncodedRevisionRef(revisionName string) (string, error) {
+	if len(revisionName) == 0 {
+		return "", fmt.Errorf("missing revision name, please provide a revision name")
+	}
+
+	encodedRevisionName := url.PathEscape(revisionName)
+	return fmt.Sprintf("revisions/%s", encodedRevisionName), nil
+}
+
+func GetEncodedVersionRef(revisionName string) (string, error) {
+	if len(revisionName) == 0 {
+		return "", fmt.Errorf("missing version name, please provide a version name")
+	}
+
+	encodedRevisionName := url.PathEscape(revisionName)
+	return fmt.Sprintf("versions/%s", encodedRevisionName), nil
+}

internal/resources/configuration/utils_test.go

@@ -0,0 +1,65 @@
+// Copyright Mia srl
+// SPDX-License-Identifier: Apache-2.0
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package configuration
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGetEncodedRef(t *testing.T) {
+	tests := []struct {
+		name         string
+		revisionName string
+		versionName  string
+		expected     string
+		expectError  string
+	}{
+		{
+			name:         "both revision and version specified",
+			revisionName: "rev1",
+			versionName:  "v1",
+			expectError:  "both revision and version specified, please provide only one",
+		},
+		{
+			name:         "only revision specified",
+			revisionName: "rev1",
+			expected:     "revisions/rev1",
+		},
+		{
+			name:        "only version specified",
+			versionName: "v1",
+			expected:    "versions/v1",
+		},
+		{
+			name:        "neither revision nor version specified",
+			expectError: "missing revision/version name, please provide one as argument",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := GetEncodedRef(tt.revisionName, tt.versionName)
+			if tt.expectError != "" {
+				require.EqualError(t, err, tt.expectError)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tt.expected, result)
+		})
+	}
+}