Improve feature.textproto path checking (openconfig#1850)

* Run Go and Protobuf checks on a schedule

* Run Go and Protobuf checks on all PRs

* Improve feature.textproto path checking

* Always validating against HEAD of OC models to improve development velocity.
* Checks are only done for added/modified feature.textproto files in PRs.
* Scheduled runs on the main branch will still check all affected
  feature.textproto files.

Tested manually.

The check now outputs logging messages when validating each file.

* try with context var

* try again with origin

* try again with another var

* fix

* try again

* should work

* fix syntax

* Add log message on how many files to validate

* Test deletion

* Add back

* Fix typo and test deletion again

* Fix bug

* add back

* Output GitHub annotations

* Test github annotations

* test

* Give annotations for dependency check

* See what happens if logging.Info

* test

* test

* test

* only modified

* test no change

* Add log message and test failure

* Test no failure

* Move log

* more log

* Fix bug

.github/workflows/protobufs.yml

@@ -90,6 +90,8 @@ jobs:
         go-version: '1.19'
     - name: Checkout code
       uses: actions/checkout@v3
+      with:
+        fetch-depth: 0
     - name: Cache
       uses: actions/cache@v2
       with:
@@ -101,5 +103,19 @@ jobs:
     - name: Fetch Openconfig Models
       run: make openconfig_public
     - name: Validate Paths
-      run: make validate_paths
+      run: |
+        # https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
+        if [ ! -z "${GITHUB_BASE_REF}" ]; then
+          readonly HEAD=${{ github.event.pull_request.head.sha }}
+          readonly BASE="$(git merge-base origin/main "${HEAD}")"
+          if ! git diff --diff-filter=D --name-only "${BASE}" | grep -E 'feature.textproto$'; then
+            # If it is a pull request AND if no feature.textproto files were
+            # deleted, then we can skip checking all but the added/modified
+            # feature.textproto files.
+            export FEATURE_FILES=changed-feature-textprotos.githubactions.txt
+            # grep: don't error out on no match.
+            git diff --diff-filter=d --name-only "${BASE}" | { grep -E 'feature.textproto$' || true; } > "${FEATURE_FILES}"
+          fi
+        fi
+        make validate_paths
 

Makefile

@@ -17,9 +17,11 @@ openconfig_public:
 .PHONY: validate_paths
 validate_paths: openconfig_public proto/feature_go_proto/feature.pb.go
 	go run -v tools/validate_paths.go \
+		-alsologtostderr \
 		--feature_root=$(CURDIR)/feature/ \
 		--yang_roots=$(CURDIR)/openconfig_public/release/models/,$(CURDIR)/openconfig_public/third_party/ \
-		--yang_skip_roots=$(CURDIR)/openconfig_public/release/models/wifi
+		--yang_skip_roots=$(CURDIR)/openconfig_public/release/models/wifi \
+		--feature_files=${FEATURE_FILES}
 
 proto/feature_go_proto/feature.pb.go: proto/feature.proto
 	mkdir -p proto/feature_go_proto

tools/clone_oc_public.sh

@@ -3,7 +3,5 @@
 set -e
 git clone https://github.com/openconfig/public.git "$1"
 cd "$1"
-# presence of "-" indicates prelease https://semver.org/#spec-item-9
-branch="$(git tag -l | grep -v "-" | sort -V | tail -1)"
-git checkout "$branch"
+# Use latest commit of OpenConfig public repo.
 echo "Using github.com/openconfig/public branch: $branch"

tools/validate_paths.go

@@ -23,6 +23,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"reflect"
 	"sort"
 	"strings"
 
@@ -44,6 +45,7 @@ var (
 	yangSkipsFlag = flag.String(
 		"yang_skip_roots", "", "sub-directories of the .yang roots which should be ignored.",
 	)
+	featureFilesFlag = flag.String("feature_files", "", "optional file containing list of feature.textprotos to validate instead of checking all files. If not specified, then all files will be checked. Note that all files will still be checked and annotated, but only these files will cause failure.")
 )
 
 var (
@@ -52,21 +54,6 @@ var (
 	skipYANGDirs = map[string]bool{}
 )
 
-func init() {
-	flag.Parse()
-	if *featuresRootFlag == "" {
-		log.Fatal("feature_root must be set.")
-	}
-	if *yangRootsFlag == "" {
-		log.Fatal("yang_roots must be set.")
-	}
-	featuresRoot = *featuresRootFlag
-	yangPaths = strings.Split(*yangRootsFlag, ",")
-	for _, s := range strings.Split(*yangSkipsFlag, ",") {
-		skipYANGDirs[s] = true
-	}
-}
-
 type pathType int
 
 const (
@@ -162,62 +149,99 @@ func modules() (map[string]*yang.Module, error) {
 }
 
 type line struct {
-	oc     string
 	line   int32
+	column int32
+	oc     string
 	detail string
 }
 
 type file struct {
 	name  string
 	lines []line
 	// Errors which are not correlated with a line.
-	errors       []string
-	dependencies []string
+	errors []string
 }
 
-// checkFiles parses all `path:` lines in the input `files`, reporting any syntax errors and paths
-// which are not in the `knownOC` set.
-func checkFiles(knownOC map[string]pathType, files []string) ([]file, error) {
-	report := []file{}
+func (f file) githubAnnotations() string {
+	var b strings.Builder
+	for _, errLine := range f.errors {
+		b.WriteString(fmt.Sprintf("::%s file=%s::%s\n", "error", f.name, errLine))
+	}
+
+	for _, line := range f.lines {
+		// https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message
+		b.WriteString(fmt.Sprintf("::%s file=%s,line=%d,col=%d::%s %s\n", "error", f.name, line.line, line.column, line.detail, line.oc))
+	}
+
+	return b.String()
+}
+
+func constructValidProfiles(files []string) (map[string]bool, map[string]*file) {
 	tmp := fppb.FeatureProfile{}
-	validProfile := make(map[string]bool)
+	validProfiles := make(map[string]bool)
+	reports := make(map[string]*file)
 
 	for _, f := range files {
+		report := reports[f]
+		if report == nil {
+			report = &file{name: f}
+			reports[f] = report
+		}
 		bs, err := os.ReadFile(f)
 		if err != nil {
 			// Just accumulate the file error since we can't do anything else.
-			report = append(report, file{name: f, errors: []string{err.Error()}})
+			report.errors = append(report.errors, err.Error())
 			continue
 		}
 
-		var errs []string
-		var dependencies []string
-
 		// Unmarshal will report syntax errors (although generally without line numbers).
 		if err := prototext.Unmarshal(bs, &tmp); err != nil {
-			errs = append(errs, err.Error())
+			report.errors = append(report.errors, err.Error())
 		}
 
 		// Validate feature profile ID name by checking path.
 		targetFeatureProfileName := getFeatureProfileNameFromPath(f)
 		featureProfileIDName := tmp.GetId().GetName()
-		validProfile[featureProfileIDName] = true
 		if targetFeatureProfileName != featureProfileIDName {
-			errs = append(errs, featureProfileIDName+" is inconsistent with path, want "+targetFeatureProfileName)
-			validProfile[featureProfileIDName] = false
+			report.errors = append(report.errors, featureProfileIDName+" is inconsistent with path, want "+targetFeatureProfileName)
+		} else {
+			validProfiles[featureProfileIDName] = true
 		}
+	}
+	return validProfiles, reports
+}
 
-		for _, dependency := range tmp.FeatureProfileDependency {
-			if dependency.GetName() != "" {
-				dependencies = append(dependencies, dependency.GetName())
-			}
+// checkFiles parses all `path:` lines in the input `files`, reporting any syntax errors and paths
+// which are not in the `knownOC` set.
+func checkFiles(knownOC map[string]pathType, files []string, validProfiles map[string]bool, reports map[string]*file) error {
+	tmp := fppb.FeatureProfile{}
+
+	log.Infof("%d files to validate", len(files))
+
+	for _, f := range files {
+		log.Infof("Validating file: %v", f)
+		report := reports[f]
+		if report == nil {
+			report = &file{name: f}
+			reports[f] = report
+		}
+
+		bs, err := os.ReadFile(f)
+		if err != nil {
+			// Just accumulate the file error since we can't do anything else.
+			report.errors = append(report.errors, err.Error())
+			continue
+		}
+
+		// Unmarshal will report syntax errors (although generally without line numbers).
+		if err := prototext.Unmarshal(bs, &tmp); err != nil {
+			report.errors = append(report.errors, err.Error())
 		}
 
 		// Use parser.Parse so I can get line numbers for OC paths we don't recognize.
-		lines := []line{}
 		ast, err := parser.Parse(bs)
 		if err != nil {
-			return nil, err
+			return err
 		}
 		for _, a := range ast {
 			switch a.Name {
@@ -239,22 +263,44 @@ func checkFiles(knownOC map[string]pathType, files []string) ([]file, error) {
 								detail = "missing from YANG"
 							}
 							if detail != "" {
-								lines = append(lines, line{
-									oc: v.Value, line: c.Start.Line, detail: detail})
+								report.lines = append(report.lines, line{
+									line:   c.Start.Line,
+									column: c.Start.Column,
+									oc:     v.Value,
+									detail: detail,
+								})
+							}
+						}
+					}
+				}
+			case "feature_profile_dependency":
+				for _, c := range a.Children {
+					if c.Name == "name" {
+						for _, v := range c.Values {
+							profileName := v.Value[1 : len(v.Value)-1] // Trim quotes
+							if !validProfiles[profileName] {
+								report.lines = append(report.lines, line{
+									line:   c.Start.Line,
+									column: c.Start.Column,
+									detail: "cannot find feature profile dependency " + profileName,
+								})
 							}
 						}
 					}
 				}
 			}
 		}
+	}
+	return nil
+}
 
-		if len(lines) == 0 && len(errs) == 0 && len(dependencies) == 0 {
-			continue
+// cleanReports removes any empty reports.
+func cleanReports(reports map[string]*file) {
+	for key, report := range reports {
+		if reflect.DeepEqual(report, &file{name: report.name}) {
+			delete(reports, key)
 		}
-		report = append(report, file{name: f, lines: lines, dependencies: dependencies, errors: errs})
 	}
-	report = validateDependency(validProfile, report)
-	return report, nil
 }
 
 // getFeatureProfileNameFromPath gets feature profile id.name from path.
@@ -265,25 +311,10 @@ func getFeatureProfileNameFromPath(file string) string {
 	return strings.Join(featureProfileFilePathArray, "_")
 }
 
-// validateDependency validates dependency from existing feature profile ID lists.
-func validateDependency(validProfile map[string]bool, reports []file) []file {
-	newReports := []file{}
-	for _, report := range reports {
-		for _, dependency := range report.dependencies {
-			if !validProfile[dependency] {
-				report.errors = append(report.errors, "can not find feature profile dependency "+dependency)
-			}
-		}
-		if len(report.lines) != 0 || len(report.errors) != 0 {
-			newReports = append(newReports, file{name: report.name, lines: report.lines, errors: report.errors})
-		}
-	}
-	return newReports
-}
-
-// featureFiles lists the file paths containing features data.
-func featureFiles() ([]string, error) {
-	files := []string{}
+// featureFiles returns the feature files to check and all existing feature files.
+func featureFiles() (map[string]struct{}, []string, error) {
+	var allFiles []string
+	filesToCheck := map[string]struct{}{}
 	err := filepath.WalkDir(featuresRoot,
 		func(path string, e fs.DirEntry, err error) error {
 			if err != nil {
@@ -293,19 +324,53 @@ func featureFiles() ([]string, error) {
 				return nil
 			}
 			if e.Name() == "feature.textproto" {
-				files = append(files, path)
+				allFiles = append(allFiles, path)
+				filesToCheck[path] = struct{}{}
 			}
 			return nil
 		})
 	if err != nil {
-		return nil, err
+		return nil, nil, err
+	}
+	sort.Strings(allFiles)
+
+	if *featureFilesFlag != "" {
+		filesToCheck = map[string]struct{}{}
+		readBytes, err := os.ReadFile(*featureFilesFlag)
+		if err != nil {
+			log.Fatalf("cannot read feature_files flag: %v", err)
+		}
+		for _, line := range strings.Split(string(readBytes), "\n") {
+			line = strings.TrimSpace(line)
+			if line == "" {
+				continue
+			}
+			path, err := filepath.Abs(line)
+			if err != nil {
+				return nil, nil, err
+			}
+			filesToCheck[path] = struct{}{}
+		}
 	}
-	sort.Strings(files)
-	return files, nil
+
+	return filesToCheck, allFiles, nil
 }
 
 // Check that every OC path used in the Feature Profiles is defined in the public OpenConfig yang.
 func main() {
+	flag.Parse()
+	if *featuresRootFlag == "" {
+		log.Fatal("feature_root must be set.")
+	}
+	if *yangRootsFlag == "" {
+		log.Fatal("yang_roots must be set.")
+	}
+	featuresRoot = *featuresRootFlag
+	yangPaths = strings.Split(*yangRootsFlag, ",")
+	for _, s := range strings.Split(*yangSkipsFlag, ",") {
+		skipYANGDirs[s] = true
+	}
+
 	ms, err := modules()
 	if err != nil {
 		log.Fatal(err)
@@ -315,22 +380,30 @@ func main() {
 		addKnownPaths(knownPaths, yang.ToEntry(m))
 	}
 
-	files, err := featureFiles()
+	filesToCheck, allFiles, err := featureFiles()
 	if err != nil {
 		log.Fatal(err)
 	}
 
-	reports, err := checkFiles(knownPaths, files)
-	if err != nil {
+	validProfiles, reports := constructValidProfiles(allFiles)
+	if err := checkFiles(knownPaths, allFiles, validProfiles, reports); err != nil {
 		log.Fatal(err)
 	}
 
+	cleanReports(reports)
+
+	log.Infof("%d files must pass: %v", len(filesToCheck), filesToCheck)
 	if len(reports) == 0 {
 		return
 	}
 
 	msg := []string{"Feature paths inconsistent with YANG schema:"}
+	failed := false
 	for _, f := range reports {
+		fmt.Print(f.githubAnnotations())
+		if _, ok := filesToCheck[f.name]; ok {
+			failed = true
+		}
 		msg = append(msg, "  file: "+f.name)
 		if len(f.errors) != 0 {
 			msg = append(msg, "  toplevel errors:")
@@ -342,6 +415,8 @@ func main() {
 			msg = append(msg, fmt.Sprintf("    line %d: %s %s", l.line, l.detail, l.oc))
 		}
 	}
-	log.Error(strings.Join(msg, "\n"))
-	os.Exit(1)
+	log.Info(strings.Join(msg, "\n"))
+	if failed {
+		os.Exit(1)
+	}
 }