initial draft of operator-testing-tool proposal for openshift-4.3 #1916
Conversation
openshift-ci-robot
added
the
size/L
|
Make sure to run the markdown checker, |
|
Actually we might have to fix the marker checker to handle the proposal format |
|
/test marker |
| - "/enhancements/our-past-effort.md" | ||
| --- | ||
|
|
||
| # operator-testing-tool |
There was a problem hiding this comment.
| # operator-testing-tool | |
| ## New Operator Testing Tool for Operator SDK |
There was a problem hiding this comment.
Also, after starting to read it will not help the users/devs impl testing. I understand that it is a tool to check its QA. So, WDYT about :
| # operator-testing-tool | |
| New Operator QA Tool for Operator SDK (Scorecard improvements and extraction) |
There was a problem hiding this comment.
I would suggest that we match the epic name directly.
| # operator-testing-tool | |
| # Tooling for Testing Operators |
There was a problem hiding this comment.
updated in the next version.
| superseded-by: | ||
| - "/enhancements/our-past-effort.md" | ||
| --- | ||
|
|
There was a problem hiding this comment.
It is not part of the template, wdyt about remove it?
Also. note that the status are not the same suggested by @joelanford here: #1929 (comment)
There was a problem hiding this comment.
@camilamacedo86 We're using this template for enhancement proposal-style proposals.
If it makes sense, we could consider moving to that template for all future proposals.
|
|
||
| A scheme for applying the labels to the tests would need to be developed as well as label names to categorize tests. | ||
|
|
||
| The “source of truth” for validation would need to be established such as which library the scorecard would use to perform CR/CSV/bundle validation. |
There was a problem hiding this comment.
CR/CRD/RBCA -> By default
OLM files (CSV) are optional - IMHO should be checked if they exist in the project and/or a flag for it be used.
Maybe semething like as: operator-sdk-qa operator --olm=false
| ### Risks and Mitigations | ||
|
|
||
| The scorecard would implement a version flag in the CLI to allow users to migrate from current functionality to the proposed functionality (e.g. v1alpha2): | ||
| * operator-sdk scorecard --version v1alpha2 |
There was a problem hiding this comment.
IMHO: Should be a goal check if the versions are following the good practices and in this way be able to check the latest one. I mean how the users should version the source should be a rule as sem version.
There was a problem hiding this comment.
This version is the version of the output produced by the scorecard. It has nothing to do with operator or CSV versions.
With respect to versioning operator source code, CRDs and CSVs, I agree that it would be nice to be able to help users understand if they're following semver, but that's definitely out-of-scope for this epic, and possibly out-of-scope for scorecard.
However one possible new test that we could think about in the future would be to round-trip of CRs between different versions to see if they result in the operator taking the same actions against the cluster.
| The exit code of scorecard would be 0 if all tests passed. The exit code would be non-zero if tests failed. With this change scores are essentially replaced with a list of pass/fail(s). | ||
|
|
||
| The scorecard would by default run all tests regardless if a single test fails. Using a CLI flag such as below would cause the test execution to stop on a failure: | ||
| * operator-sdk scorecard -l ‘testgroup=required’ --fail-fast |
There was a problem hiding this comment.
Since we will no longer have scores I think we should change the name in order to make it more intuitive. I think it should be something as operator-sdk project-qa.. I am not good with names... however... something that shows that the propose of this is to check the QA of the project.
There was a problem hiding this comment.
I think the name "scorecard" still holds since we're still running a bunch of generic black-box tests and reporting to developers (and eventually users, I think) about which tests passed and failed.
| Tests can fall into 3 groups: required, recommended, or optional. Tests can also be categorized as static or runtime. Labels for these groups and categories would allow a test to be more precisely specified by an end user. | ||
|
|
||
| Possible examples of specifying what tests to run are as follows: | ||
| * operator-sdk scorecard -l ‘testgroup in (required,recommended,optional)’ |
There was a problem hiding this comment.
In the practice what will be the diff between recommended and optional? In my POV that all NO mandatory recommendations are Optional. WDYT?
IMHO: It should be mandatory and recommended.
There was a problem hiding this comment.
If the optional is related to the tests impl by the devs when they extending its API and creates their plugins I think we could give a name more intuitive as plugins or additional
There was a problem hiding this comment.
I agree. @dmesser can you describe what the difference is between recommended and optional?
As a user, why would I want to run optional, but not recommended, tests?
Do we have a breakdown of which of the current tests fall into these two/three catagories?
If not, I think we should propose what we think and circulate with the OLM and CVP teams for feedback.
| Possible examples of specifying what tests to run are as follows: | ||
| * operator-sdk scorecard -l ‘testgroup in (required,recommended,optional)’ | ||
| * operator-sdk scorecard -l ‘testtype in (runtime,static)’ | ||
| * operator-sdk scorecard -l updatecrtest=true |
There was a problem hiding this comment.
What does it means?
- (runtime,static)
- updatecrtest
There was a problem hiding this comment.
I think these are just examples of syntax for selecting tests based on labels.
testtype in (runtime,static)would mean any test where thetesttypelabel is either"runtime"OR"static"updatecrtest=truewould mean any test where theupdatecrtestlabel is"true"
The labels are arbitrary, so these label examples will not necessarily be the ones we define on the tests.
| Rough point estimate for this story is 8 points. | ||
| #### Story 3 - Common Validation | ||
| The scorecard would use a common validation codebase to verify bundle contents. This story supports a single “source of truth” and avoids duplication or variance of validation logic. The scorecard user would be able to specify a bundle on the command line, that bundle would then be validated. For example: | ||
| * operator-sdk scorecard --bundle ./my-operator |
There was a problem hiding this comment.
--config I think is better ( bundle is nomenclature added in the community repo which for me at least was not intuitive at all)
There was a problem hiding this comment.
IIRC, --config is already used to define the config file used by the scorecard. I think --bundle is good because "bundle" has a well-defined definition in the context of the Operator Framework and it's what we're expecting to be passed to this flag.
One caveat/question: @dmesser Are we expecting to test the entire set of bundles that make up a channel/catalog, or are we scoping the scorecard to a single bundle at a time?
| Tasks for this story include: | ||
| * determine the correct validation library to use | ||
| * add support for a bundle directory flag to the CLI | ||
| * change the scorecard code to use that validation library when validating a CR, CSV, or given a bundle directory |
There was a problem hiding this comment.
IMHO: It should be:
- CR/CRD/RBCA -> By default
- OLM files (CSV) are optional - IMHO should be checked if they exist in the project and/or a flag for it be used.
Note: The bundle in POV is not an intuitive nomenclature.
There was a problem hiding this comment.
I think one issue with the scorecard today is that it has too much configuration complexity. The following flags can all be used to define how scorecard runs. Some are optional, some are required, and some are conditionally required. There are also interdependencies between them that can cause confusion.
--basic-tests Enable basic operator checks (default true)
--cr-manifest strings Path to manifest for Custom Resource (required) (specify flag multiple times for multiple CRs)
--crds-dir string Directory containing CRDs (all CRD manifest filenames must have the suffix 'crd.yaml') (default "deploy/crds")
--csv-path string Path to CSV being tested
--global-manifest string Path to manifest for Global resources (e.g. CRD manifests)
--namespace string Namespace of custom resource created in cluster
--namespaced-manifest string Path to manifest for namespaced resources (e.g. RBAC and Operator manifest)
--olm-deployed The OLM has deployed the operator. Use only the CSV for test data
--olm-tests Enable OLM integration checks (default true)
If we go all-in on OLM integration (which I believe is a goal), then I think we should require a bundle and leverage other OLM integrations (e.g. installing OLM and running an operator with OLM) so that we can reduce configuration complexity and make running and troubleshooting scorecard much more straightforward.
@dmesser @shawn-hurley WDYT?
|
|
||
| #### Story 4 - Update scorecard Documentation | ||
| Tasks: | ||
| * Document changes to CLI |
There was a problem hiding this comment.
IMHO We should not have the cli commands documented in the docs instead of it we should have a --help command and some automatically doc as it was done here for example using Makefile.
If we will use cobra then it is possible to get done as well.
There was a problem hiding this comment.
I think it should be both, document the changes but we need to make sure the help text is accurate.
There was a problem hiding this comment.
I believe the CLI library we use supports generating docs from the CLI configuration, and I think it's in our backlog to investigate using that to keep our SDK CLI reference up-to-date. I could see this being something we contribute to upstream and then inherit as part of our Kubebuilder integration plan.
| * Document changes to CLI | ||
| * Document new output formats | ||
| * Document changes to configuration | ||
| * Document breaking changes and removals |
There was a problem hiding this comment.
| * Document breaking changes and removals | |
| * update the CHANGELOG and Migration files with breaking changes and removals |
| Tasks: | ||
| * Document changes to CLI | ||
| * Document new output formats | ||
| * Document changes to configuration |
There was a problem hiding this comment.
* Update doc X of the scorecard with the changes ( also IMHO it should be done for each change performed, I mean in the same PR instead of having a task to do all at the end. Maybe we can have a task to review/improve the doc, after all, be implemented. )
``
|
|
||
| ### User Stories | ||
|
|
||
| #### Story 1 - Show pass/fail in Scorecard Output |
There was a problem hiding this comment.
For all user story, IMHO we need to update them in order to follow its common definitions as
As a of the product, ## title
I can
So that .
Following an example for actions:
- A user can post her resume to the web site.
- A user can search for jobs.
- A company can post new job openings.
- A user can limit who can see her résumé.
S few refs [1](https://www.berteig.com/how-to-apply-agile/summary-of-user-stories-the-three-cs-and-invest/ and) and 2
Also,
Shows that the task for each user stories is broken already Tasks for this story include:. I would replace it for Acceptance Criteria Instead of.
There was a problem hiding this comment.
@dmesser has been pushing this point as well. I don't think we should require this method of writing stories in the proposals this time around because it wasn't discussed as a team, and there are still some kinks to work out with how these things are translated into JIRA issues.
Let's talk about this as a team for the 4.4+ epics and planning.
|
|
||
| ## Design Details | ||
|
|
||
| ### Test Plan |
There was a problem hiding this comment.
Have we not a default process to impl all features? Should it really be added here in the Proposal doc? IMHO it shows out of the scope of this doc. Also, it shows not part of this topic. I'd add things like that in the section ### Observations and open questions of its template.
There was a problem hiding this comment.
This is part of the enhancement proposal template, hence it's inclusion. I think test planning is a relevant and important aspect of incorporating new features.
| These are generalized examples to consider, in addition to the aforementioned | ||
| maturity levels. | ||
|
|
||
| ##### Dev Preview -> Tech Preview |
There was a problem hiding this comment.
IMHO this doc recommends we change the features of the scorecard, so:
- I'd adopted the
incremental development processwhere each deliverable change with its docs would be merge in the master. (In this way we can add value to the users and if the scope change in the middle will not require to start from the scratch / agile process ) - In the end, if we still think that it should be another tool and etc .. we could extract the impl for a new repo/project
| subproject, repos requested, github details, and/or testing infrastructure. | ||
|
|
||
| Listing these here allows the community to get the process for these resources | ||
| started right away. |
There was a problem hiding this comment.
| started right away. |
I think it is not required to be added here too.
There was a problem hiding this comment.
This is boilerplate from the template. Once we have consensus on the contents of the proposal, I think it's safe to remove the sections that are not applicable to this proposal.
| **For non-optional features moving to GA, the graduation criteria must include | ||
| end to end tests.** | ||
|
|
||
| ##### Removing a deprecated feature |
There was a problem hiding this comment.
I think it is not required.
- operator-sdk still in init dev. See that its tag is < 1.0.0
- In POV the biggest part of users has not been using it anyway
- The CI in the community-operators repos will be affected as well but after a new release with the changes, it should be still working fine by using the previous version until we are able to update there. Maybe is required to signalize who is responsible for this repo about our intention for they are able to follow up and plan the changes.
There was a problem hiding this comment.
While we're not yet to 1.0.0, the CVP has automation around scorecard that would break suddenly if we didn't have a deprecation and removal plan that was communicated in advance. In this case, I think it's important to document what we're doing:
- Adding a new
--versionflag - Adding a new result output spec,
v1alpha2, based on the changes described in this proposal. - Setting the default
--versiontov1alpha1to avoid breaking existing CLI users. - Deprecating
v1alpha1, and informing users to move tov1alpha2by setting--version=v1alpha2on the command line. - In a subsequent release, removing support for
v1alpha1.
There was a problem hiding this comment.
I'll update this section in a new version of the proposal.
|
|
||
|
|
||
|
|
||
| ### Implementation Details/Notes/Constraints |
There was a problem hiding this comment.
This feels like the proposal is not implementable.
- How are we going to create a labeling system? Does a user provide their own list of labels for a test? do we provide that list? do we provide an initial list?
Are we planing on writing a DSL of some sort to parse this? testtype in (runtime,static) are we worried that we are signing up for more than we bargained for?
There was a problem hiding this comment.
Can we add that info to the Implementation Details of the proposal?
There was a problem hiding this comment.
I'll update the proposal and include that list in Implementation Details.
| #### Story 1 - Show pass/fail in Scorecard Output | ||
| Today, the scorecard output shows a percentage of tests that were successful to the end user. This story is to change the scorecard output to show a pass or fail for each test that is run in the output instead of a success percentage. | ||
|
|
||
| The exit code of scorecard would be 0 if all tests passed. The exit code would be non-zero if tests failed. With this change scores are essentially replaced with a list of pass/fail(s). |
There was a problem hiding this comment.
Can you expand on this to document the discussion we had about how the exit code should only apply to required tests?
There was a problem hiding this comment.
will update in a new version of the proposal.
Description of the change:
this is the proposal draft document for the operator-testing-tool, for openshift-4.3
Motivation for the change:
not a breaking change or code fix, just the proposal for a new feature.