adding required annotations check and necessary tests #1086

acornett21 · 2023-10-19T20:12:45Z

Adding a new check that checks for newly required annotations. At first we will only warn partners about this, then after sometime we will move to enforce this check.

Relates: JIRA: EET-3540

Note: There are backend changes being made to support Warning being in the results

openshift-ci · 2023-10-19T20:12:52Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dcibot · 2023-10-19T21:01:33Z

FAILURE https://www.distributed-ci.io/jobs/04d95d8e-1033-4ed4-bc74-a7e360657d1c/jobStates
from adding required annotations check and necessary tests #1086

tkrishtop · 2023-10-19T21:04:56Z

Hi @acornett21 the DCI job failed because of a new test you've added

TASK [redhatci.ocp.verify_tests : Fail if not all test results are as expected for preflight_operator_simple-demo-operator_results-junit.xml]
0s
task path: /usr/share/ansible/collections/ansible_collections/redhatci/ocp/roles/verify_tests/tasks/parse_junit_file.yml:35
fatal: [jumphost]: FAILED! => 
{"changed": false, "msg": "The following expectations failed: 
[{'failed_expectation': {'testcase': '[a-zA-Z]+', 'passed': True}, 
'actual_result': {'testcase': 'RequiredAnnotations', 'passed': False}}]"}

I'll change the expectations for simple-demo-operator tomorrow - for the moment we simply expect all tests to be green.

bcrochet

A few comments.

internal/formatters/util.go

internal/policy/operator/required_annotations.go

internal/formatters/util.go

internal/formatters/junitxml_test.go

acornett21 · 2023-10-20T13:57:26Z

I'll change the expectations for simple-demo-operator tomorrow - for the moment we simply expect all tests to be green.

@tkrishtop Are you saying the new check showed up as failed? If so that's incorrect. This is still WIP, and I'll be working to update simple-demo-operator and simple-disconnected-operator accordingly, once this feature is fully hashed out.

internal/csv/csv.go

internal/policy/operator/required_annotations.go

komish · 2023-10-20T14:48:42Z

internal/csv/csv.go

+	TokenAuthAWSAnnotation           = "features.operators.openshift.io/token-auth-aws"
+	TokenAuthAzureAnnotation         = "features.operators.openshift.io/token-auth-azure"
+	TokenAuthGCPAnnotation           = "features.operators.openshift.io/token-auth-gcp"


This is a potentially unrelated question.. but why are these becoming required? Are we installing operators on all providers all at once?

I'm not sure if a given operator would/could support token auth for all cloud providers or not. My guess would be that is unlikely, just based on our dealings with STS in the ACK project. IMO It's more so for the below:

The current format of the infrastructure annotations is unsuited for robust checks and effective enforcement. We need a new format that allows to cleanly differentiate between mere absence of a data or actual lack of support for a certain infrastructure feature.

dcibot · 2023-10-20T14:55:47Z

SUCCESS https://www.distributed-ci.io/jobs/6fa21747-3eb5-4cbe-acbd-177e128e07c5/jobStates
from https://github.com/dci-labs/dallas-pipelines/pull/911

dcibot · 2023-10-23T09:48:41Z

SUCCESS https://www.distributed-ci.io/jobs/fc238d3b-40e4-4fc0-9b17-7e4448bec34a/jobStates
from https://github.com/dci-labs/dallas-pipelines/pull/911

dcibot · 2023-10-23T18:18:58Z

FAILURE https://www.distributed-ci.io/jobs/e33a353a-c880-4c82-b0b0-8307748d0771/jobStates
from adding required annotations check and necessary tests #1086

dcibot · 2023-10-23T22:28:58Z

FAILURE https://www.distributed-ci.io/jobs/b5469f1b-c770-4c81-ab7d-28c16a164ef1/jobStates
from adding required annotations check and necessary tests #1086

tkrishtop · 2023-10-24T09:07:46Z

I adjusted the pinned tests on our side, to ignore the new test failing for simple-demo. For the future, it would be nice to fix the simple-demo as well.

tkrishtop · 2023-10-24T09:07:55Z

check workload preflight-green

tkrishtop · 2023-10-24T09:17:06Z

I'll change the expectations for simple-demo-operator tomorrow - for the moment we simply expect all tests to be green.

@tkrishtop Are you saying the new check showed up as failed? If so that's incorrect. This is still WIP, and I'll be working to update simple-demo-operator and simple-disconnected-operator accordingly, once this feature is fully hashed out.

Sorry @acornett21 , I just saw your comment. Would you prefer me to fallback the fix on our side and pin all the tests for the simple-demo to be green, as before?

With the old setup, the DCI job for your PR will be failing because it automatically auto-discovers all the tests and expects them to be green:

      - filename: 'preflight_operator_simple-demo-operator_results-junit.xml'
        expected_results:
          - testcase: '[a-zA-Z]+'
            passed: True

dcibot · 2023-10-24T09:37:01Z

FAILURE https://www.distributed-ci.io/jobs/fbc562fe-75b1-4782-8f58-d65983c4bf91/jobStates
from adding required annotations check and necessary tests #1086

acornett21 · 2023-10-24T16:29:22Z

@tkrishtop I've updated the the junit code to add another level/tag (not sure what to call this for junit). But the output will look like this for checks that are at a warn level.

<testsuites>
	<testsuite tests="5" failures="0" warnings="1" time="35.215827" name="Red Hat Certification">
		<properties></properties>
		<testcase classname="quay.io/opdev/simple-demo-operator-bundle:v0.0.6" name="ScorecardBasicSpecCheck" time="3.516921">Check to make sure that all CRs have a spec block.</testcase>
		<testcase classname="quay.io/opdev/simple-demo-operator-bundle:v0.0.6" name="ScorecardOlmSuiteCheck" time="3.530103">Operator-sdk scorecard OLM Test Suite Check</testcase>
		<testcase classname="quay.io/opdev/simple-demo-operator-bundle:v0.0.6" name="DeployableByOLM" time="28.148170">Checking if the operator could be deployed by OLM</testcase>
		<testcase classname="quay.io/opdev/simple-demo-operator-bundle:v0.0.6" name="ValidateOperatorBundle" time="0.010793">Validating Bundle image that checks if it can validate the content and format of the operator bundle</testcase>
		<testcase classname="quay.io/opdev/simple-demo-operator-bundle:v0.0.6" name="RequiredAnnotations" time="9.839866ms">
			<warning message="Warn" type="">Check that the CSV has all of the required feature annotations.: Suggested Fix: Add all of the required annotations, and make sure the value is set to either &#39;true&#39; or &#39;false&#39;</warning>
		</testcase>
	</testsuite>
</testsuites>

The idea of introducing a new check at level warn is so that partners can see the check in their results, but not be penalized if it fails. Then at a later date the check will be moved to level best where the check is enforced. I hope this clears up what we are trying to accomplish. Let me know if we need to adjust the XML, or if you see any issue in how the XML works, since I think DCI is the only consumer of this.

dcibot · 2023-10-24T16:54:54Z

FAILURE https://www.distributed-ci.io/jobs/084ea93c-3924-40dc-bb99-47becbdc8ec9/jobStates
from adding required annotations check and necessary tests #1086

dcibot · 2023-10-24T17:26:30Z

FAILURE https://www.distributed-ci.io/jobs/c5be0b64-9302-4aae-a700-b30eeb40afb2/jobStates
from adding required annotations check and necessary tests #1086

dcibot · 2023-11-09T19:05:40Z

SUCCESS https://www.distributed-ci.io/jobs/de9ade0f-5c61-46df-937c-e46f4f2edb6c/jobStates
from adding required annotations check and necessary tests #1086

acornett21 · 2023-11-15T15:37:53Z

/ok-to-test

dcibot · 2023-11-17T21:20:00Z

SUCCESS https://www.distributed-ci.io/jobs/32501c48-f325-41ff-a794-0868f5d90ed6/jobStates
from adding required annotations check and necessary tests #1086

acornett21 · 2023-11-17T21:35:08Z

/ok-to-test

acornett21 · 2023-11-20T17:21:10Z

Tested locally as well now that the operators are updated...

{
    "image": "quay.io/opdev/simple-demo-operator-bundle:v0.0.7",
    "passed": true,
    "certification_hash": "d41d8cd98f00b204e9800998ecf8427e",
    "test_library": {
        "name": "github.com/redhat-openshift-ecosystem/openshift-preflight",
        "version": "0.0.0",
        "commit": "ebe3811bfac4353f96a706e9dd30329946b4f07b"
    },
    "results": {
        "passed": [
            {
                "name": "ScorecardBasicSpecCheck",
                "elapsed_time": 2666,
                "description": "Check to make sure that all CRs have a spec block."
            },
            {
                "name": "ScorecardOlmSuiteCheck",
                "elapsed_time": 3548,
                "description": "Operator-sdk scorecard OLM Test Suite Check"
            },
            {
                "name": "DeployableByOLM",
                "elapsed_time": 42394,
                "description": "Checking if the operator could be deployed by OLM"
            },
            {
                "name": "ValidateOperatorBundle",
                "elapsed_time": 51,
                "description": "Validating Bundle image that checks if it can validate the content and format of the operator bundle"
            },
            {
                "name": "RequiredAnnotations",
                "elapsed_time": 12,
                "description": "Checks that the CSV has all of the required feature annotations."
            }
        ],
        "failed": [],
        "errors": []
    }
}

bcrochet

A few requests.

bcrochet · 2023-12-11T14:43:46Z

internal/engine/engine.go

@@ -236,6 +236,12 @@ func (c *craneEngine) ExecuteChecks(ctx context.Context) error {
 		}

 		if !checkPassed {
+			// if a test doesn't pass but is of level warn include it in warning results, instead of failed results
+			if check.Metadata().Level == "warn" {


This is different... You use "warning" on 223, but "warn" here. Perhaps time for a constant for these levels?

I moved this to a constant, though I'm not sure the package is the ideal place. I put it in check.certification which defines the Check interface.

internal/engine/engine_test.go

internal/policy/operator/required_annotations_test.go

internal/policy/operator/restricted_network_aware.go

dcibot · 2023-12-11T19:13:26Z

SUCCESS https://www.distributed-ci.io/jobs/bd700a34-3fe9-46db-a4ab-6494fcfc838c/jobStates
from adding required annotations check and necessary tests #1086

dcibot · 2023-12-11T22:02:45Z

SUCCESS https://www.distributed-ci.io/jobs/dbba234e-f7f5-4bf1-93ac-3c388d3d22d8/jobStates
from adding required annotations check and necessary tests #1086

dcibot · 2023-12-12T15:21:29Z

SUCCESS https://www.distributed-ci.io/jobs/2878be73-a4ae-4f29-841e-51c36f03f405/jobStates
from adding required annotations check and necessary tests #1086

internal/engine/engine.go

bcrochet

Notes about the import name

bcrochet · 2023-12-12T17:36:33Z

internal/engine/engine.go

@@ -220,7 +220,7 @@ func (c *craneEngine) ExecuteChecks(ctx context.Context) error {
 		c.results.TestedImage = c.image

 		logger.V(log.DBG).Info("running check", "check", check.Name())
-		if check.Metadata().Level == "optional" {
+		if check.Metadata().Level == "optional" || check.Metadata().Level == certcheck.LevelWarn {


Why not use the Optional const?

To keep this PR focused on the required annotations, I was planning a follow up PR to switch all other checks to the constants. I assumed that would be preference then introducing here. But maybe it's fine to just introduce for this one line since it is already changing?

I switched this one line, but as mentioned another pr will follow to update all the things.

internal/engine/engine.go

Signed-off-by: Adam D. Cornett <[email protected]>

komish

Barring E2E, LGTM.

/lgtm

bcrochet

/lgtm

internal/engine/engine.go

openshift-ci · 2023-12-12T18:06:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, komish

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:

~~OWNERS~~ [acornett21,bcrochet,komish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dcibot · 2023-12-12T18:34:47Z

SUCCESS https://www.distributed-ci.io/jobs/255347a4-36bd-44d0-a75e-746b0ffec0ad/jobStates
from adding required annotations check and necessary tests #1086

acornett21 requested review from bcrochet and komish October 19, 2023 20:12

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 19, 2023

bcrochet requested changes Oct 20, 2023

View reviewed changes

openshift-ci bot assigned bcrochet Oct 20, 2023

komish reviewed Oct 20, 2023

View reviewed changes

internal/csv/csv.go Outdated Show resolved Hide resolved

komish reviewed Oct 20, 2023

View reviewed changes

internal/policy/operator/required_annotations.go Outdated Show resolved Hide resolved

komish reviewed Oct 20, 2023

View reviewed changes

openshift-ci bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 15, 2023

acornett21 marked this pull request as ready for review December 11, 2023 14:40

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2023

openshift-ci bot requested review from skattoju and tonytcampbell December 11, 2023 14:40

bcrochet requested changes Dec 11, 2023

View reviewed changes

komish reviewed Dec 12, 2023

View reviewed changes

internal/engine/engine.go Outdated Show resolved Hide resolved

bcrochet requested changes Dec 12, 2023

View reviewed changes

adding required annotations check and necessary tests

16711f1

Signed-off-by: Adam D. Cornett <[email protected]>

komish approved these changes Dec 12, 2023

View reviewed changes

openshift-ci bot assigned komish Dec 12, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2023

bcrochet approved these changes Dec 12, 2023

View reviewed changes

internal/engine/engine.go Show resolved Hide resolved

acornett21 merged commit 147224e into redhat-openshift-ecosystem:main Dec 12, 2023
1 check passed

adding required annotations check and necessary tests #1086

adding required annotations check and necessary tests #1086

Conversation

acornett21 commented Oct 19, 2023

openshift-ci bot commented Oct 19, 2023

dcibot commented Oct 19, 2023

tkrishtop commented Oct 19, 2023 • edited Loading

bcrochet left a comment

Choose a reason for hiding this comment

acornett21 commented Oct 20, 2023

komish Oct 20, 2023

Choose a reason for hiding this comment

acornett21 Oct 20, 2023

Choose a reason for hiding this comment

dcibot commented Oct 20, 2023

dcibot commented Oct 23, 2023

dcibot commented Oct 23, 2023

dcibot commented Oct 23, 2023

tkrishtop commented Oct 24, 2023

tkrishtop commented Oct 24, 2023

tkrishtop commented Oct 24, 2023

dcibot commented Oct 24, 2023

acornett21 commented Oct 24, 2023

dcibot commented Oct 24, 2023

dcibot commented Oct 24, 2023

dcibot commented Nov 9, 2023

acornett21 commented Nov 15, 2023

dcibot commented Nov 17, 2023

acornett21 commented Nov 17, 2023

acornett21 commented Nov 20, 2023 • edited Loading

bcrochet left a comment

Choose a reason for hiding this comment

bcrochet Dec 11, 2023

Choose a reason for hiding this comment

acornett21 Dec 11, 2023

Choose a reason for hiding this comment

dcibot commented Dec 11, 2023

dcibot commented Dec 11, 2023

dcibot commented Dec 12, 2023

bcrochet left a comment

Choose a reason for hiding this comment

bcrochet Dec 12, 2023

Choose a reason for hiding this comment

acornett21 Dec 12, 2023

Choose a reason for hiding this comment

acornett21 Dec 12, 2023

Choose a reason for hiding this comment

komish left a comment

Choose a reason for hiding this comment

bcrochet left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 12, 2023

dcibot commented Dec 12, 2023

tkrishtop commented Oct 19, 2023 •

edited

Loading

acornett21 commented Nov 20, 2023 •

edited

Loading