-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: features metadata #3287
feat: features metadata #3287
Conversation
Skipping CI for Draft Pull Request. |
@mlavacca would you mind adding a docs update to the description on conformance testing so users know what they are getting themselves into with this change? Looks like every time you add a new conformance test there are quite a few places to change. Since there's no GEP - why did you decide to make changes for all features and not just for experimental features? And my complaint - every time we make sweeping changes to conformance testing, we lose the skills and attention of people with experience using the previous implementation, thence potentially losing that team/skillset. |
Not really. This PR adds some metadata to the features but does not introduce any change in how conformance tests are written. In order to create a new conformance test, we just need to set the feature name as part of the test. We need to perform a new step when defining new features (instead of just defining the feature name, we need to create a new feature struct as well). I'll ensure docs mention this aspect 👍 This is the first time we are trying to introduce a lifecycle concept on the features.
It's not possible to make this change only for a subset of features. We are defining metadata on features, and I really think we need to do so for all our features, instead of having different mechanisms based on the feature maturity.
This is very related to the first point. If one wants to add a new conformance test to an already defined feature, nothing changes. In case a new feature has to be defined, the feature definition implies the creation of a struct instead of just the creation of a string. It does not seems that we are adding implementors-facing complexity to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mlavacca!
pkg/features/feature.go
Outdated
|
||
// TODO: comment | ||
const ( | ||
FeatureStatusTrial = "TRIAL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I think that we actually need two separate levels here:
- The stability of the feature (the max release channel this has made it to)
- The stability of the test (tests that are brand new should default to a lower level of stability than tests that have been around for a release cycle or two with multiple passing implementations)
I think it's useful to solve both of these, and this PR seems to be on a path to solve the first one. With that said, if we're agreeing that this PR is primarily targeting the first item, then I'd use the existing "Experimental" and "Standard" terminology for each feature.
Solving 2 may actually be easier. I think it just becomes a marker on tests, and implementations have the option to skip these tests when generating a report without any kind of penalty in their conformance report (or something along those lines).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I agree with the distinction and the fact that this PR is suitable to address the first point. As you pointed out, the 2nd point is easier, as it can be done via a specific field on the test struct.
I'll change the feature status names accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done the change and marked the PR as ready for review, PTAL :)
71ecef0
to
552e3dd
Compare
Thanks @mlavacca! /lgtm |
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mlavacca, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Does this PR introduce a user-facing change?: