Delete feature.textproto

[dplore]

This proposes to deprecate the feature.textproto files from this repository.

These files were intended to assert some list of paths that were convenient for people to refer to when describing some feature of a network device. These would in turn guide the organization of functional/ondatra tests to be created.

In practice, we document the intended OC paths to be tested in README. The feature.textprotos are sometimes maintained and other times not so they are not kept up to date. Having two places in the repository which are maintained by hand leads to problems with keeping things in sync.

A proposed step to aid in ensuring the OC paths specified in README are formatted properly and are valid is to use a YAML format embedded in the readme which could be automatically parsed and checked. See #2746 for an example work in progress.

[OpenConfigBot]

Pull Request Functional Test Report for #2793 / 524efaf

No tests identified for validation.

Help

[coveralls]

Pull Request Test Coverage Report for Build 11813971511

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 55.268%

---

Totals
Change from base Build 11813015596:	0.0%
Covered Lines:	1983
Relevant Lines:	3588

---

💛 - Coveralls

[bensons]

Personal opinion: The FeatureProfile is useful for explicitly identifying the branch points for features / subfeatures / sub-subfeatures etc, as well as providing metadata about each given Feature. This use is separable from whether or not the FeatureProfile is also used for tracking OC paths.

[bstoll]

I generally think it sounds less than ideal for the official mechanism for determining if a path is covered by a test is "parse the readme markdown, look for section X, parse the body of the section as yaml, marshal it into proto paths, and validate the paths exist".

Regardless, the contents of feature.textproto is actively validated against OC models. The tooling around this would need to be removed if this is being deprecated:

featureprofiles/Makefile

Lines 17 to 24 in 8856fd8

	.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 \
	--feature_files=${FEATURE_FILES}

featureprofiles/README.md

Lines 178 to 181 in 8856fd8

	# Path validation
	The `make validate_paths` target will clone the public OpenConfig definitions
	and report Feature Profiles that have invalid OpenConfig paths.

featureprofiles/.github/workflows/protobufs.yml

Lines 87 to 123 in 8856fd8

	validate_oc_paths:
	name: Validate OpenConfig Paths
	runs-on: ubuntu-latest
	steps:
	- name: Install go
	uses: actions/setup-go@v2
	with:
	go-version: '1.21'
	- name: Checkout code
	uses: actions/checkout@v3
	with:
	fetch-depth: 0
	- name: Cache
	uses: actions/cache@v3
	with:
	path: |
	~/go/pkg/mod
	~/.cache/go-build
	key: ${{ github.job }}-${{ runner.os }}-go-build-${{ hashFiles('**/go.sum') }}
	- name: Fetch Openconfig Models
	run: make openconfig_public
	- name: 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

And the validation code in tools/validate_paths.go

[bensons]

It seems to me that test authors should document a subset of the total paths exercised in a test, as a way to indicate which paths are specific to the Feature (vs setting up context for the test). I believe there is no good way to know that programmatically, unless the author specifies it.

It's also true that, in many cases, configuration of a Feature requires a set of paths which is greater than any one test might exercise. (example: BGP, counter-example: NTP) And, like tests, there is no good way to know programmatically which paths are essential to the Feature, unless they are specified by somebody who is an expert in the Feature. (If it were possible to say something simple like "Feature X requires all paths in this branch / model / whatever" then that might not be true, but that doesn't seem to be the case for many features today.)

So, I think that a) we need feature.textproto to include a set of paths that are essential to the Feature, and b) we need a PR check to ensure that all paths in a README are also included in the feature.textproto for the Feature being tested. It's possible to use (b) to automatically add paths to feature.textproto, and perhaps thus auto-generate it. But if that's the only way that paths are populated in feature.textproto then it wouldn't necessarily possess the quality that I described in the 2nd paragraph.

[wenovus]

If the goal of featureprofiles is to document the set of paths that belongs to a feature, then that is necessarily manually curated.

As Brandon pointed out validate_paths.go and any existing tooling that depends on feature.textproto will break, and so deleting feature.textproto will mean we need to migrate all such tooling to parse YAML.

I think an easier way to have a single SoT for features is to have a tool that parses the proposed YAML and auto-populates feature.textproto so that no tooling will break. A CI check can be created to make sure feature.textproto is kept up-to-date.

So I think the right steps are 1. Decide where to store the manual SoT (source-of-truth) for feature paths, 2. Start creating them (e.g. README/YAML), 3. Start to auto-gen feature.textproto where the new SoT has been defined for feature paths.

On the other hand, if the longer term proposal is rather to move the proto-format documentation of feature paths to a different place (metadata.textproto?), then we would still need to migrate such tooling, but I don't see a significant advantage to doing that because at the end of the day the difference is two proto files vs. one proto file.

[bensons]

@wenovus proposal seems reasonable, with one caveat: if feature.textproto paths are populated by merging the paths from all subtending tests (e.g. READMEs) then it's unclear whether feature.textproto can be reliable as a Feature definition; feature.textproto would only be complete if the Feature has complete path coverage by subtending tests.

I think you're correct to say that CI should verify that README paths are also in feature.textproto, and it's probably reasonable to say that missing paths should be auto-added to feature.textproto. But I would stop short of saying that feature.textproto is completely autogenerated. Rather, it should be seen as a curated list, with some human ownership.

[bensons]

Following up on my previous comment, and proposing details in response to this:

So I think the right steps are 1. Decide where to store the manual SoT (source-of-truth) for feature paths, 2. Start creating them (e.g. README/YAML), 3. Start to auto-gen feature.textproto where the new SoT has been defined for feature paths.

1. SoT for a Feature is feature.textproto and SoT for a Test is README.md
   • Workflow for a Test developer is to add Covered Paths to README.md
   • Workflow for a "Feature Maintainer" is to curate feature.textproto, ensuring it includes all paths that are required to exercise the Feature
2. We should agree on the YAML structure first, and then update all READMEs to move the Covered Paths (etc) to that format; structure details TBD
3. Rather than "auto-gen feature.textproto" I would instead say "auto-add each Test's Covered Paths and RPCs to feature.textproto"

[wenovus]

Hmm thanks for pointing out the caveat, I agree with your analysis. If we want to manually curate the overall list of paths within a feature so that it's independent of whether test coverage is complete, then your revised proposal and its details makes sense to me.

However, I want to see if I can challenge this: do we want to manually curate the overall list, or should it simply be the union of "intended Covered Paths" listed by all READMEs within a feature? Such that we implicitly have the following policy: "if a test is not defined, then its covered paths are not part of the featureprofile."?

Doing this means we guarantee that each featureprofile path is mapped to a test, whether proposed or implemented. It also avoids the maintenance burden of having to manually maintain the overall list of feature paths -- only the covered paths in each constituent test needs to be maintained as proposed by OP.

[bensons]

Thanks for your response @wenovus.

First, I would assert that we do need a single list of mandatory paths for any given Feature, so that implementations (e.g. vendors) have a clear definition of compliance. Eventually that compliance may be described as "just pass all the tests", but that would be inadequate until we know that the tests are completely covering a Feature's paths. And how do we know that a Feature has all of its paths covered by Tests? Today, feature.textproto is that definition of coverage compliance.

More specifically responding to your challenge, I would ask: what is the roadmap for Test coverage (Test development)? I think that the desired end-state is for a collection of subtending Tests to exercise all of the paths that are required to implement a Feature, but initially there are no tests...

In my view, a human-curated feature.textproto might be used as a roadmap which indicates all of the paths that MUST be implemented for a Feature and SHOULD be covered by subtending Tests. If we adopt the policy that you described ("if a test is not defined, then its covered paths are not part of the featureprofile") then would we, instead, need to have all of those paths defined in a "roadmap README" for each Feature until they are included in specific test READMEs? That seems to be effectively the same as just documenting them in the feature.textproto in the first place. 🤓

[wenovus]

Thanks @bensons, having feature.textproto partially manually-maintained in order to record paths in the roadmap makes sense to me. Having a roadmap README to maintain would create a similar maintenance burden until the roadmap README can be deleted. I think all my questions are addressed, so I'll let the author and other reviewers chime in.

[robshakir]

I agree that the discussions above are important -- but I'm not clear that these files are actually doing anything today (or are updated regularly!).

@dplore's proposal #2863 seems to give us a way to define paths that are required for a test that will be related to that test specifically. It is also closer to the test such that we can say "this test said it should cover path X but it doesn't! ⚠️"
more easily. So, Darren's proposed approach seems preferable to me.

Adding LGTM for that reason. 👍🏼

[robshakir]

@dplore - this looks like can be merged. Any concerns with doing so?

[bstoll]

I'm not sure if proto/feature.proto should be removed as well (along with the related proto/feature_go_proto and build rules for it)? If we remove all the feature.textproto entries, there are no users of feature.proto in this repo any longer.