diff --git a/fixtures/sites/valid_both_string_and_int_key.yml b/fixtures/sites/valid_both_string_and_int_key.yml new file mode 100644 index 0000000..b66c1ee --- /dev/null +++ b/fixtures/sites/valid_both_string_and_int_key.yml @@ -0,0 +1,7 @@ +--- +api_version: 1 +domain_maps: + dev: + 1: dev-srtest.pantheonsite.io + "2": about.dev-srtest.pantheonsite.io + "3": employee-resources.dev-srtest.pantheonsite.io \ No newline at end of file diff --git a/fixtures/sites/valid_string_as_key.yml b/fixtures/sites/valid_string_as_key.yml new file mode 100644 index 0000000..1c4abc6 --- /dev/null +++ b/fixtures/sites/valid_string_as_key.yml @@ -0,0 +1,7 @@ +--- +api_version: 1 +domain_maps: + dev: + "30": dev-srtest.pantheonsite.io + "2": about.dev-srtest.pantheonsite.io + "3": employee-resources.dev-srtest.pantheonsite.io \ No newline at end of file diff --git a/pkg/model/sites.go b/pkg/model/sites.go index 9dd84f6..9c47581 100644 --- a/pkg/model/sites.go +++ b/pkg/model/sites.go @@ -10,4 +10,9 @@ type SitesYml struct { type DomainMaps map[string]DomainMapByEnvironment // DomainMapByEnvironment is a collection of site domains keyed by site ID. -type DomainMapByEnvironment map[int]string +// +// Given an int is valid in yaml but not json, and it is difficult to validate +// if a key is an int or string in PHP (c/f terminus-yml-validator-plugin), it +// is easiest to unmarshal into a string, and check that the key is also a +// valid site ID as part of the validator. +type DomainMapByEnvironment map[string]string diff --git a/pkg/validator/sites.go b/pkg/validator/sites.go index b3bb2f2..1b80fdd 100644 --- a/pkg/validator/sites.go +++ b/pkg/validator/sites.go @@ -5,6 +5,7 @@ import ( "os" "regexp" "sites-yml-validator/pkg/model" + "strconv" "gopkg.in/yaml.v3" ) @@ -64,7 +65,10 @@ func validateDomainMaps(domainMaps map[string]model.DomainMapByEnvironment) erro if domainMapCount > maxDomainMaps { return fmt.Errorf("%q has too many domains listed (%d). Maximum is %d", env, domainMapCount, maxDomainMaps) } - for _, domain := range domainMap { + for siteID, domain := range domainMap { + if !isValidSiteID(siteID) { + return fmt.Errorf("%q is not a valid site ID", siteID) + } if !validHostname.MatchString(domain) { return fmt.Errorf("%q is not a valid hostname", domain) } @@ -81,3 +85,22 @@ func validateAPIVersion(apiVersion int) error { } return nil } + +func isValidSiteID(s string) bool { + // I don't know that "" will be possible unmarshalling real yaml, but covering our bases + if s == "" { + return false + } + // Either value of 0 (not a valid site id), or leading zeros (which would strconv to drop the 0) + // is problematic for consistent handling in the job runner script. + if s[0:1] == "0" { + return false + } + // Is the string an int? + i, err := strconv.Atoi(s) + if err != nil { + return false + } + // Is the number positive? + return i > 0 +} diff --git a/pkg/validator/sites_test.go b/pkg/validator/sites_test.go index c2ec507..1053daa 100644 --- a/pkg/validator/sites_test.go +++ b/pkg/validator/sites_test.go @@ -49,16 +49,16 @@ func TestValidate(t *testing.T) { APIVersion: 1, DomainMaps: model.DomainMaps{ "dev": model.DomainMapByEnvironment{ - 1: "site1.dev-mysite.pantheonsite.io", + "1": "site1.dev-mysite.pantheonsite.io", }, "test": model.DomainMapByEnvironment{ - 1: "site1.dev-mysite.pantheonsite.io", + "1": "site1.dev-mysite.pantheonsite.io", }, "live": model.DomainMapByEnvironment{ - 1: "site1.mysite.com", + "1": "site1.mysite.com", }, "autopilot": model.DomainMapByEnvironment{ - 1: "site1.autopilot-mysite.pantheonsite.io", + "1": "site1.autopilot-mysite.pantheonsite.io", }, }, }, @@ -70,10 +70,10 @@ func TestValidate(t *testing.T) { APIVersion: 1, DomainMaps: model.DomainMaps{ "dev": model.DomainMapByEnvironment{ - 1: "site1.dev-mysite.pantheonsite.io", + "1": "site1.dev-mysite.pantheonsite.io", }, "mylongmultidevname": model.DomainMapByEnvironment{ - 1: "site1.mylongmultidevname-mysite.pantheonsite.io", + "1": "site1.mylongmultidevname-mysite.pantheonsite.io", }, }, }, @@ -85,10 +85,10 @@ func TestValidate(t *testing.T) { APIVersion: 1, DomainMaps: model.DomainMaps{ "dev": model.DomainMapByEnvironment{ - 1: "site1.dev-mysite.pantheonsite.io", + "1": "site1.dev-mysite.pantheonsite.io", }, "feat_branch": model.DomainMapByEnvironment{ - 1: "site1.feat-branch-mysite.pantheonsite.io", + "1": "site1.feat-branch-mysite.pantheonsite.io", }, }, }, @@ -100,38 +100,38 @@ func TestValidate(t *testing.T) { APIVersion: 1, DomainMaps: model.DomainMaps{ "dev": model.DomainMapByEnvironment{ - 1: "site1.dev-mysite.pantheonsite.io", - 2: "site2.dev-mysite.pantheonsite.io", - 3: "site3.dev-mysite.pantheonsite.io", - 4: "site4.dev-mysite.pantheonsite.io", - 5: "site5.dev-mysite.pantheonsite.io", - 6: "site6.dev-mysite.pantheonsite.io", - 7: "site7.dev-mysite.pantheonsite.io", - 8: "site8.dev-mysite.pantheonsite.io", - 9: "site9.dev-mysite.pantheonsite.io", - 10: "site10.dev-mysite.pantheonsite.io", - 11: "site11.dev-mysite.pantheonsite.io", - 12: "site12.dev-mysite.pantheonsite.io", - 13: "site13.dev-mysite.pantheonsite.io", - 14: "site14.dev-mysite.pantheonsite.io", - 15: "site15.dev-mysite.pantheonsite.io", - 16: "site16.dev-mysite.pantheonsite.io", - 17: "site17.dev-mysite.pantheonsite.io", - 18: "site18.dev-mysite.pantheonsite.io", - 19: "site19.dev-mysite.pantheonsite.io", - 20: "site20.dev-mysite.pantheonsite.io", - 21: "site21.dev-mysite.pantheonsite.io", - 22: "site22.dev-mysite.pantheonsite.io", - 23: "site23.dev-mysite.pantheonsite.io", - 24: "site24.dev-mysite.pantheonsite.io", - 25: "site25.dev-mysite.pantheonsite.io", - 26: "site26.dev-mysite.pantheonsite.io", - 27: "site27.dev-mysite.pantheonsite.io", - 28: "site28.dev-mysite.pantheonsite.io", - 29: "site29.dev-mysite.pantheonsite.io", + "1": "site1.dev-mysite.pantheonsite.io", + "2": "site2.dev-mysite.pantheonsite.io", + "3": "site3.dev-mysite.pantheonsite.io", + "4": "site4.dev-mysite.pantheonsite.io", + "5": "site5.dev-mysite.pantheonsite.io", + "6": "site6.dev-mysite.pantheonsite.io", + "7": "site7.dev-mysite.pantheonsite.io", + "8": "site8.dev-mysite.pantheonsite.io", + "9": "site9.dev-mysite.pantheonsite.io", + "10": "site10.dev-mysite.pantheonsite.io", + "11": "site11.dev-mysite.pantheonsite.io", + "12": "site12.dev-mysite.pantheonsite.io", + "13": "site13.dev-mysite.pantheonsite.io", + "14": "site14.dev-mysite.pantheonsite.io", + "15": "site15.dev-mysite.pantheonsite.io", + "16": "site16.dev-mysite.pantheonsite.io", + "17": "site17.dev-mysite.pantheonsite.io", + "18": "site18.dev-mysite.pantheonsite.io", + "19": "site19.dev-mysite.pantheonsite.io", + "20": "site20.dev-mysite.pantheonsite.io", + "21": "site21.dev-mysite.pantheonsite.io", + "22": "site22.dev-mysite.pantheonsite.io", + "23": "site23.dev-mysite.pantheonsite.io", + "24": "site24.dev-mysite.pantheonsite.io", + "25": "site25.dev-mysite.pantheonsite.io", + "26": "site26.dev-mysite.pantheonsite.io", + "27": "site27.dev-mysite.pantheonsite.io", + "28": "site28.dev-mysite.pantheonsite.io", + "29": "site29.dev-mysite.pantheonsite.io", }, "mdev": model.DomainMapByEnvironment{ - 1: "site1.mdev-mysite.pantheonsite.io", + "1": "site1.mdev-mysite.pantheonsite.io", }, }, }, @@ -143,18 +143,33 @@ func TestValidate(t *testing.T) { APIVersion: 1, DomainMaps: model.DomainMaps{ "dev": model.DomainMapByEnvironment{ - 1: "site1.dev-mysite.pantheonsite.io", + "1": "site1.dev-mysite.pantheonsite.io", }, "test": model.DomainMapByEnvironment{ - 1: "$(sudo do something dangerous)", + "1": "$(sudo do something dangerous)", }, "live": model.DomainMapByEnvironment{ - 1: "site1.mysite.com", + "1": "site1.mysite.com", }, }, }, errors.New(`"$(sudo do something dangerous)" is not a valid hostname`), }, + { + "invalid site id", + model.SitesYml{ + APIVersion: 1, + DomainMaps: model.DomainMaps{ + "dev": model.DomainMapByEnvironment{ + "foo": "site1.dev-mysite.pantheonsite.io", + }, + "test": model.DomainMapByEnvironment{ + "foo": "site1.test-mysite.pantheonsite.io", + }, + }, + }, + errors.New(`"foo" is not a valid site ID`), + }, } { t.Run(tc.name, func(t *testing.T) { v, err := ValidatorFactory("sites") @@ -232,6 +247,8 @@ func TestValidateSitesFromFilePath(t *testing.T) { "error reading YAML file: open ../../fixtures/sites/this_file_does_not_exist.yml: no such file or directory", ), }, + {"valid_string_as_key", nil}, + {"valid_both_string_and_int_key", nil}, } { t.Run(tc.fixtureName, func(t *testing.T) { v, err := ValidatorFactory("sites") @@ -249,3 +266,29 @@ func TestValidateSitesFromFilePath(t *testing.T) { }) } } + +func TestIsValidSiteID(t *testing.T) { + for _, tc := range []struct { + input string + expected bool + }{ + {"1", true}, + {"300", true}, + {"foo", false}, + {"i", false}, + {"1a", false}, + {"", false}, + {"1.2", false}, + {".2", false}, + {"0.2", false}, + {"1.0", false}, + {"0", false}, + {"00", false}, + {"01", false}, + {"-5", false}, + } { + t.Run(tc.input, func(t *testing.T) { + assert.Equal(t, tc.expected, isValidSiteID(tc.input)) + }) + } +}