Handle the old key when writing configuration, but write the new one

internal/controlplane/handlers_providers_test.go

@@ -164,13 +164,32 @@ func TestCreateProvider(t *testing.T) {
 			expected: minder.Provider{
 				Name: "test-github-app-defaults",
 				Config: newPbStruct(t, map[string]interface{}{
-					"github-app": map[string]interface{}{},
+					"github_app": map[string]interface{}{},
+				}),
+				Class: string(db.ProviderClassGithubApp),
+			},
+		},
+		{
+			name:          "test-github-app-config-newkey",
+			providerClass: db.ProviderClassGithubApp,
+			userConfig: newPbStruct(t, map[string]interface{}{
+				"github_app": map[string]interface{}{
+					"key":      "value", // will be ignored
+					"endpoint": "my.little.github",
+				},
+			}),
+			expected: minder.Provider{
+				Name: "test-github-app-config-newkey",
+				Config: newPbStruct(t, map[string]interface{}{
+					"github_app": map[string]interface{}{
+						"endpoint": "my.little.github",
+					},
 				}),
 				Class: string(db.ProviderClassGithubApp),
 			},
 		},
 		{
-			name:          "test-github-app-config",
+			name:          "test-github-app-config-oldkey",
 			providerClass: db.ProviderClassGithubApp,
 			userConfig: newPbStruct(t, map[string]interface{}{
 				"github-app": map[string]interface{}{
@@ -179,9 +198,9 @@ func TestCreateProvider(t *testing.T) {
 				},
 			}),
 			expected: minder.Provider{
-				Name: "test-github-app-config",
+				Name: "test-github-app-config-oldkey",
 				Config: newPbStruct(t, map[string]interface{}{
-					"github-app": map[string]interface{}{
+					"github_app": map[string]interface{}{
 						"endpoint": "my.little.github",
 					},
 				}),

internal/providers/github/clients/app.go

@@ -113,7 +113,8 @@ func NewGitHubAppProvider(
 // embedding the struct to expose its JSON tags
 type appConfigWrapper struct {
 	*minderv1.ProviderConfig
-	GitHubApp *minderv1.GitHubAppProviderConfig `json:"github-app" yaml:"github-app" mapstructure:"github-app" validate:"required"`
+	GitHubAppOldKey *minderv1.GitHubAppProviderConfig `json:"github-app" yaml:"github-app" mapstructure:"github-app"`
+	GitHubApp       *minderv1.GitHubAppProviderConfig `json:"github_app" yaml:"github_app" mapstructure:"github_app"`
 }
 
 // ParseV1AppConfig parses the raw config into a GitHubAppProviderConfig struct
@@ -138,15 +139,31 @@ func ParseV1AppConfig(rawCfg json.RawMessage) (
 		}
 	}
 
-	return w.ProviderConfig, w.GitHubApp, nil
+	// we used to have a required key on the gh app config, so we need to check both
+	// until we migrate and can remove the old key
+	ghAppConfig := w.GitHubApp
+	if ghAppConfig == nil {
+		ghAppConfig = w.GitHubAppOldKey
+	}
+	if ghAppConfig == nil {
+		return nil, nil, fmt.Errorf("no GitHub App config found")
+	}
+
+	return w.ProviderConfig, ghAppConfig, nil
+}
+
+// embedding the struct to expose its JSON tags
+type appConfigWrapperWrite struct {
+	*minderv1.ProviderConfig
+	GitHubApp *minderv1.GitHubAppProviderConfig `json:"github_app" yaml:"github_app" mapstructure:"github_app" validate:"required"`
 }
 
 // MarshalV1AppConfig marshals the GitHubAppProviderConfig struct into a raw config
 func MarshalV1AppConfig(
 	providerCfg *minderv1.ProviderConfig,
 	appCfg *minderv1.GitHubAppProviderConfig,
 ) (json.RawMessage, error) {
-	w := appConfigWrapper{
+	w := appConfigWrapperWrite{
 		ProviderConfig: providerCfg,
 		GitHubApp:      appCfg,
 	}

internal/providers/github/clients/app_test.go

@@ -50,7 +50,19 @@ func TestParseV1AppConfig(t *testing.T) {
 		provEvalFn func(*testing.T, *minderv1.ProviderConfig)
 	}{
 		{
-			name:   "valid app config",
+			name:   "valid app config - new key",
+			config: json.RawMessage(`{ "github_app": { "endpoint": "https://api.github.com" } }`),
+			ghEvalFn: func(t *testing.T, ghConfig *minderv1.GitHubAppProviderConfig) {
+				t.Helper()
+				assert.Equal(t, "https://api.github.com", ghConfig.Endpoint)
+			},
+			provEvalFn: func(t *testing.T, providerConfig *minderv1.ProviderConfig) {
+				t.Helper()
+				assert.Nil(t, providerConfig)
+			},
+		},
+		{
+			name:   "valid app config - old key",
 			config: json.RawMessage(`{ "github-app": { "endpoint": "https://api.github.com" } }`),
 			ghEvalFn: func(t *testing.T, ghConfig *minderv1.GitHubAppProviderConfig) {
 				t.Helper()
@@ -61,6 +73,18 @@ func TestParseV1AppConfig(t *testing.T) {
 				assert.Nil(t, providerConfig)
 			},
 		},
+		{
+			name:   "valid app config - both keys prefers the new one",
+			config: json.RawMessage(`{ "github_app": { "endpoint": "https://new.github.com" }, "github-app": { "endpoint": "https://old.github.com" } }`),
+			ghEvalFn: func(t *testing.T, ghConfig *minderv1.GitHubAppProviderConfig) {
+				t.Helper()
+				assert.Equal(t, "https://new.github.com", ghConfig.Endpoint)
+			},
+			provEvalFn: func(t *testing.T, providerConfig *minderv1.ProviderConfig) {
+				t.Helper()
+				assert.Nil(t, providerConfig)
+			},
+		},
 		{
 			name:   "valid app and provider config",
 			config: json.RawMessage(`{ "auto_registration": { "entities": { "repository": {"enabled": true} } }, "github-app": { "endpoint": "https://api.github.com" } }`),
@@ -91,7 +115,7 @@ func TestParseV1AppConfig(t *testing.T) {
 		},
 		{
 			name:   "missing required github key",
-			config: json.RawMessage(`{ "auto_registration": { "entities": { "blah": {"enabled": true} } } }`),
+			config: json.RawMessage(`{ "auto_registration": { "entities": { "repository": {"enabled": true} } } }`),
 			ghEvalFn: func(t *testing.T, ghConfig *minderv1.GitHubAppProviderConfig) {
 				t.Helper()
 				assert.Nil(t, ghConfig)
@@ -100,7 +124,7 @@ func TestParseV1AppConfig(t *testing.T) {
 				t.Helper()
 				assert.Nil(t, providerConfig)
 			},
-			error: "Field validation for 'GitHubApp' failed on the 'required' tag",
+			error: "no GitHub App config found",
 		},
 	}
 

internal/providers/github/service/service.go

@@ -449,7 +449,7 @@ func (_ *ghProviderService) GetConfig(
 	case db.ProviderClassGithub:
 		defaultConfig = `{"github": {}}`
 	case db.ProviderClassGithubApp:
-		defaultConfig = `{"github-app": {}}`
+		defaultConfig = `{"github_app": {}}`
 	default:
 		return nil, fmt.Errorf("unsupported provider class %s", class)
 	}