From d6b1088f565f454f16fad1e2d9bc746777f087e5 Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Fri, 15 Mar 2024 11:27:41 -0700 Subject: [PATCH] chore(internal/postprocessor): add validation command and CI (#9575) --- .github/.OwlBot.yaml | 2 +- .github/workflows/owlbot_validation.yml | 24 +++ internal/postprocessor/README.md | 32 ++++ internal/postprocessor/config.go | 77 +++++---- internal/postprocessor/config.yaml | 3 - internal/postprocessor/main.go | 11 ++ internal/postprocessor/validate.go | 217 ++++++++++++++++++++++++ 7 files changed, 333 insertions(+), 33 deletions(-) create mode 100644 .github/workflows/owlbot_validation.yml create mode 100644 internal/postprocessor/validate.go diff --git a/.github/.OwlBot.yaml b/.github/.OwlBot.yaml index 81ceb87e4cdf..4dc6ae6ec012 100644 --- a/.github/.OwlBot.yaml +++ b/.github/.OwlBot.yaml @@ -70,7 +70,7 @@ deep-remove-regex: - /cloudcontrolspartner/apiv1/ - /clouddms/apiv1/ - /cloudprofiler/apiv2/ - - /cloudquotas/v1/ + - /cloudquotas/apiv1/ - /cloudtasks/apiv2/ - /cloudtasks/apiv2beta2/ - /cloudtasks/apiv2beta3/ diff --git a/.github/workflows/owlbot_validation.yml b/.github/workflows/owlbot_validation.yml new file mode 100644 index 000000000000..b616e2ccbb6f --- /dev/null +++ b/.github/workflows/owlbot_validation.yml @@ -0,0 +1,24 @@ +--- +name: owlbot_validation +on: + pull_request: + paths: + - 'internal/postprocessor/config.yaml' + - '.github/.OwlBot.yaml' + +permissions: + contents: read + +jobs: + validate: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 1 + - uses: actions/setup-go@v5 + with: + go-version: 1.21.x + - run: | + git clone -b master --single-branch --depth=1 https://github.com/googleapis/googleapis.git + go run ./internal/postprocessor validate -googleapis-dir=./googleapis diff --git a/internal/postprocessor/README.md b/internal/postprocessor/README.md index d6d0db1a285e..009cc3c36dcb 100644 --- a/internal/postprocessor/README.md +++ b/internal/postprocessor/README.md @@ -82,3 +82,35 @@ The post-processor initializes new modules by generating the required files To add a new module, add the directory name of the module to `modules` in `google-cloud-go/internal/postprocessor/config.yaml`. Please maintain alphabetical ordering of the module names. + +## Validating your config changes + +The `validate` command is run as a presubmit on changes to either the +`.github/.OwlBot.yaml` or the `internal/postprocessor/config.yaml`. + +If you want to run it manually, from the **repository root**, simply run the +following: + +``` +go run ./internal/postprocessor validate +``` + +If you want to validate existence of service config yaml in the PostProcessor +config, provide an absolute path to a local clone of `googleapis`: + +``` +go run ./internal/postprocessor validate -googleapis-dir=$GOOGLEAPIS +``` + +If you want validate a specific config file, not the repository default, then +provide aboslute paths to either or both config files like so: + +``` +go run ./internal/postprocessor \ + -owl-bot-config=$OWL_BOT_YAML \ + -processor-config=$CONFIG_YAML +``` + +If you think there is an issue with the validator, just fix it in the same CL +as the config change that triggered it. No need to update the postprocessor sha +when the validate command is changed, it runs from HEAD of the branch. \ No newline at end of file diff --git a/internal/postprocessor/config.go b/internal/postprocessor/config.go index 2b84e6ea1c69..1835fd03f48b 100644 --- a/internal/postprocessor/config.go +++ b/internal/postprocessor/config.go @@ -38,6 +38,29 @@ type config struct { ManualClientInfo []*ManifestEntry } +type serviceConfigEntry struct { + InputDirectory string `yaml:"input-directory"` + ServiceConfig string `yaml:"service-config"` + ImportPath string `yaml:"import-path"` + RelPath string `yaml:"rel-path"` + ReleaseLevelOverride string `yaml:"release-level-override"` +} + +type postProcessorConfig struct { + Modules []string `yaml:"modules"` + ServiceConfigs []*serviceConfigEntry `yaml:"service-configs"` + ManualClients []*ManifestEntry `yaml:"manual-clients"` +} + +type deepCopyConfig struct { + Source string `yaml:"source"` + Dest string `yaml:"dest"` +} +type owlBotConfig struct { + DeepCopyRegex []deepCopyConfig `yaml:"deep-copy-regex"` + DeepRemoveRegex []string `yaml:"deep-remove-regex"` +} + // libraryInfo contains information about a GAPIC client. type libraryInfo struct { // ImportPath is the Go import path for the GAPIC library. @@ -54,46 +77,42 @@ type libraryInfo struct { ReleaseLevel string } -func (p *postProcessor) loadConfig() error { - var postProcessorConfig struct { - Modules []string `yaml:"modules"` - ServiceConfigs []*struct { - InputDirectory string `yaml:"input-directory"` - ServiceConfig string `yaml:"service-config"` - ImportPath string `yaml:"import-path"` - RelPath string `yaml:"rel-path"` - ReleaseLevelOverride string `yaml:"release-level-override"` - } `yaml:"service-configs"` - ManualClients []*ManifestEntry `yaml:"manual-clients"` - } - b, err := os.ReadFile(filepath.Join(p.googleCloudDir, "internal", "postprocessor", "config.yaml")) +func loadConfigs(ppcPath, obcPath string) (*postProcessorConfig, *owlBotConfig, error) { + var ppc postProcessorConfig + b, err := os.ReadFile(ppcPath) if err != nil { - return err + return nil, nil, err } - if err := yaml.Unmarshal(b, &postProcessorConfig); err != nil { - return err - } - var owlBotConfig struct { - DeepCopyRegex []struct { - Source string `yaml:"source"` - Dest string `yaml:"dest"` - } `yaml:"deep-copy-regex"` + if err := yaml.Unmarshal(b, &ppc); err != nil { + return nil, nil, err } - b2, err := os.ReadFile(filepath.Join(p.googleCloudDir, ".github", ".OwlBot.yaml")) + var obc owlBotConfig + b2, err := os.ReadFile(obcPath) if err != nil { - return err + return nil, nil, err } - if err := yaml.Unmarshal(b2, &owlBotConfig); err != nil { + if err := yaml.Unmarshal(b2, &obc); err != nil { + return nil, nil, err + } + + return &ppc, &obc, nil +} + +func (p *postProcessor) loadConfig() error { + ppcPath := filepath.Join(p.googleCloudDir, "internal", "postprocessor", "config.yaml") + obcPath := filepath.Join(p.googleCloudDir, ".github", ".OwlBot.yaml") + ppc, obc, err := loadConfigs(ppcPath, obcPath) + if err != nil { return err } c := &config{ - Modules: postProcessorConfig.Modules, + Modules: ppc.Modules, ClientRelPaths: make([]string, 0), GoogleapisToImportPath: make(map[string]*libraryInfo), - ManualClientInfo: postProcessorConfig.ManualClients, + ManualClientInfo: ppc.ManualClients, } - for _, v := range postProcessorConfig.ServiceConfigs { + for _, v := range ppc.ServiceConfigs { c.GoogleapisToImportPath[v.InputDirectory] = &libraryInfo{ ServiceConfig: v.ServiceConfig, ImportPath: v.ImportPath, @@ -101,7 +120,7 @@ func (p *postProcessor) loadConfig() error { ReleaseLevel: v.ReleaseLevelOverride, } } - for _, v := range owlBotConfig.DeepCopyRegex { + for _, v := range obc.DeepCopyRegex { i := strings.Index(v.Source, "/cloud.google.com/go") li, ok := c.GoogleapisToImportPath[v.Source[1:i]] if !ok { diff --git a/internal/postprocessor/config.yaml b/internal/postprocessor/config.yaml index 34b9d5fc65f8..99186997e9e9 100644 --- a/internal/postprocessor/config.yaml +++ b/internal/postprocessor/config.yaml @@ -979,9 +979,6 @@ service-configs: - input-directory: google/devtools/cloudbuild/v2 service-config: cloudbuild_v2.yaml import-path: cloud.google.com/go/cloudbuild/apiv2 - - input-directory: google/cloud/compute/v1 - service-config: compute_v1.yaml - import-path: cloud.google.com/go/compute/apiv1 - input-directory: google/cloud/confidentialcomputing/v1 service-config: confidentialcomputing_v1.yaml import-path: cloud.google.com/go/confidentialcomputing/apiv1 diff --git a/internal/postprocessor/main.go b/internal/postprocessor/main.go index c8dfac1b92db..d7fcce527e11 100644 --- a/internal/postprocessor/main.go +++ b/internal/postprocessor/main.go @@ -70,6 +70,17 @@ func main() { githubUsername := flag.String("gh-user", "googleapis", "GitHub username where repo lives.") prFilepath := flag.String("pr-file", "/workspace/new_pull_request_text.txt", "Path at which to write text file if changing PR title or body.") + if len(os.Args) > 1 { + switch os.Args[1] { + case "validate": + log.Println("Starting config validation.") + if err := validate(); err != nil { + log.Fatal(err) + } + log.Println("Validation complete.") + return + } + } flag.Parse() ctx := context.Background() diff --git a/internal/postprocessor/validate.go b/internal/postprocessor/validate.go new file mode 100644 index 000000000000..ed6d0831afb9 --- /dev/null +++ b/internal/postprocessor/validate.go @@ -0,0 +1,217 @@ +// Copyright 2024 Google LLC +// +// 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 main + +import ( + "errors" + "flag" + "fmt" + "log" + "os" + "path/filepath" + "regexp" + "strings" +) + +var ( + validateCmd *flag.FlagSet + owlBotConfigPath string + processorConfigPath string + validateGoogleapisDir string + + lastSegmentExecptions = map[string]bool{ + "admin": true, // firestore + "common": true, // oslogin + "type": true, // shopping et al. + "autogen": true, // longrunning + "longrunning": true, + } + moduleAPIVersionRegex = regexp.MustCompile(`\/api(v[1-9]+[a-z0-9]*)`) + directoryAPIVersionRegex = regexp.MustCompile(`\/(v[1-9]+[a-z0-9]*)`) +) + +const ( + defaultOwlBotConfig = ".github/.OwlBot.yaml" + defaultProcessorConfig = "internal/postprocessor/config.yaml" +) + +func init() { + validateCmd = flag.NewFlagSet("validate", flag.ExitOnError) + validateCmd.StringVar(&owlBotConfigPath, "owl-bot-config", "", "Absolute path to OwlBot config. Defaults to $PWD/"+defaultOwlBotConfig) + validateCmd.StringVar(&processorConfigPath, "processor-config", "", "Absolute path to PostProcessor config. Defaults to $PWD/"+defaultProcessorConfig) + validateCmd.StringVar(&validateGoogleapisDir, "googleapis-dir", "", "Absolute path to googleapis directory - enables file existence check(s). Default disabled.") +} + +func validate() error { + validateCmd.Parse(os.Args[2:]) + dir, err := os.Getwd() + if err != nil { + return err + } + + if owlBotConfigPath == "" { + owlBotConfigPath = filepath.Join(dir, defaultOwlBotConfig) + } + if processorConfigPath == "" { + processorConfigPath = filepath.Join(dir, defaultProcessorConfig) + } + log.Println("owl-bot-config set to", owlBotConfigPath) + log.Println("processor-config set to", processorConfigPath) + log.Println("googleapis-dir set to", validateGoogleapisDir) + + ppc, obc, err := loadConfigs(processorConfigPath, owlBotConfigPath) + if err != nil { + return err + } + + if err := validatePostProcessorConfig(ppc); err != nil { + log.Println("error validating post processor config") + return err + } + + if err := validateOwlBotConfig(obc, ppc); err != nil { + log.Println("error validating OwlBot config") + return err + } + + return nil +} + +func validatePostProcessorConfig(ppc *postProcessorConfig) error { + // Verify no duplicate module entries - `modules` property in YAML. + mods := make(map[string]bool, len(ppc.Modules)) + for _, m := range ppc.Modules { + if seen := mods[m]; seen { + return fmt.Errorf("duplicate post-processor modules entry: %s", m) + } + mods[m] = true + } + + serviceConfigs := make(map[string]*serviceConfigEntry) + for _, s := range ppc.ServiceConfigs { + if strings.Contains(s.InputDirectory, "grafeas") { + // Skip grafeas because it's an oddity that won't change anytime soon. + continue + } + + // Verify no duplicate service config entries by `import-path`. + if _, seen := serviceConfigs[s.ImportPath]; seen { + return fmt.Errorf("duplicate post-processor service-configs entry for import-path: %s", s.ImportPath) + } + if err := validateServiceConfigEntry(s); err != nil { + return err + } + + serviceConfigs[s.ImportPath] = s + } + + return nil +} + +func validateServiceConfigEntry(s *serviceConfigEntry) error { + if !strings.HasPrefix(s.ImportPath, "cloud.google.com/go/") { + return fmt.Errorf("import-path should start with 'cloud.google.com/go/': %s", s.ImportPath) + } + + // Verify that import-path ends with "apiv" suffix. + importMatches := moduleAPIVersionRegex.FindAllStringSubmatch(s.ImportPath, 1) + last := s.ImportPath[strings.LastIndex(s.ImportPath, "/")+1:] + if len(importMatches) == 0 && !lastSegmentExecptions[last] { + return fmt.Errorf("import-path should have an api version in format 'apiv[a-b1-9]+': %s", s.ImportPath) + } + + // Verify that input-directory ends with version suffix. + dirMatches := directoryAPIVersionRegex.FindAllStringSubmatch(s.InputDirectory, -1) + last = s.InputDirectory[strings.LastIndex(s.InputDirectory, "/")+1:] + if len(dirMatches) == 0 && !lastSegmentExecptions[last] { + return fmt.Errorf("import-path should have an api version in format 'v[a-b1-9]+': %s", s.InputDirectory) + } + + // Verify import-path version matches api version in input-directory. + // Skip this if there were no matches for the expected segments. + if len(dirMatches) > 0 && len(importMatches) > 0 { + importVersion := importMatches[0][1] + dirVersion := dirMatches[0][1] + if importVersion != dirVersion { + return fmt.Errorf("mismatched input-directory (%s) and import-path (%s) versions: %s vs. %s", s.InputDirectory, s.ImportPath, dirVersion, importVersion) + } + } + + // Verify that the service-config file actually exists, if requested. + if validateGoogleapisDir != "" { + serviceConfigPath := filepath.Join(validateGoogleapisDir, s.InputDirectory, s.ServiceConfig) + if _, err := os.Stat(serviceConfigPath); errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("service-config file does not exist: %s", serviceConfigPath) + } + } + + return nil +} + +func validateOwlBotConfig(obc *owlBotConfig, ppc *postProcessorConfig) error { + // Collect all API directories with post processor configs to ensure each + // has an appropriate OwlBot config. + postProcessedDirectories := make(map[string]*serviceConfigEntry, len(ppc.ServiceConfigs)) + postProcessedImportPaths := make(map[string]*serviceConfigEntry, len(ppc.ServiceConfigs)) + for _, s := range ppc.ServiceConfigs { + postProcessedDirectories[s.InputDirectory] = s + importPath := s.ImportPath + if s.RelPath != "" { + importPath = filepath.Join("cloud.google.com/go", s.RelPath) + } + postProcessedImportPaths[importPath] = s + } + + sources := make(map[string]bool, len(obc.DeepCopyRegex)) + for _, dcr := range obc.DeepCopyRegex { + // Verify no duplicate DeepCopyRegex configs + if sources[dcr.Source] { + return fmt.Errorf("duplicate deep-copy-regex entry: %s", dcr.Source) + } + + // Verify that each DeepCopyRegex has a corresponding PostProcessor config + // entry. Also detects if there is typo in the DeepCopyRegex source. + // + // Substring from 1 to trim the leading '/' from Source. + apiSource := dcr.Source[1:strings.Index(dcr.Source, "/cloud.google.com")] + if _, ok := postProcessedDirectories[apiSource]; !ok { + return fmt.Errorf("copied directory is missing a post-processor config or vice versa: %s", dcr.Source) + } + + sources[dcr.Source] = true + } + + removals := make(map[string]bool, len(obc.DeepRemoveRegex)) + for _, drr := range obc.DeepRemoveRegex { + drr = strings.TrimSuffix(drr, "/") + // Verify no duplicate deep-remove-regex entries. + if removals[drr] { + return fmt.Errorf("duplicate deep-remove-regex entry: %s", drr) + } + + // Verify deep-remove-regex is associated with a PostProcessor config entry. + // Also detects if there is typo in the deep-remove-regex entry. + if !strings.HasPrefix(drr, "/internal/generated/snippets") { + fullImportPath := filepath.Join("cloud.google.com/go", drr) + if _, ok := postProcessedImportPaths[fullImportPath]; !ok { + return fmt.Errorf("removed importpath is missing a post-processor config or vice versa: %s", drr) + } + } + + removals[drr] = true + } + + return nil +}