Add "alpha" feature gate tests (integration & examples) to our CI #3917

ghost · 2021-05-06T17:38:15Z

Changes

Prior to this commit the integration tests run against PRs were only those for "stable" features.

This PR adds additional integration test runs for "alpha" features as well. test/e2e-scripts.sh now patches the feature-flags ConfigMap before executing integration tests and examples tests, first with "enable-api-fields: stable" and then with "enable-api-fields: alpha".

As part of switching on alpha tests two issues have also been fixed with alpha features:

Sidecar workspaces could collide with existing volumeMounts attached
to the sidecar if the mountpath of the workspace matched an existing
volumeMount. This has been fixed by preventing the additional workspace
volumeMount if one already exists at that mountpath.
Tekton bundles integration tests needed to be updated to account
for validation errors that are returned when a bundle is invalid.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

Pipelines now e2e tests every commit against both the "stable" and "alpha" feature gates.

ghost · 2021-05-06T19:05:31Z

/kind misc

ghost · 2021-05-06T20:27:09Z

pkg/workspace/apply.go

+
+// addSidecarVolumeMount adds a volumeMount to the sidecar unless its MountPath would conflict
+// with another of the sidecar's existing volume mounts.
+func addSidecarVolumeMount(sidecar *v1beta1.Sidecar, volumeMount corev1.VolumeMount) {


This fixes a bug discovered by running the alpha e2e tests! 😎

tekton-robot · 2021-05-06T20:27:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	100.0%	97.4%	-2.6

tekton-robot · 2021-05-06T20:33:04Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	100.0%	97.4%	-2.6

tekton-robot · 2021-05-06T20:34:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/workspace/apply.go	100.0%	97.4%	-2.6

ghost · 2021-05-07T14:00:01Z

@pierretasci the tekton bundles integration tests are now being run as part of our CI (and failing atm). Is this something that should work straight away or is there more setup than just a feature flag required to get them running successfully?

I'm looking into it but wondered if you might immediately know what needs to change.

Here are the error messages:

tektonbundles_test.go:386: Error waiting for PipelineRun to finish with expected error: "hello-world-piplinerun" completed with the wrong reason: CouldntGetPipeline (message: Error retrieving pipeline for pipelinerun arendelle-25kgl/hello-world-piplinerun: error when listing pipelines for pipelineRun hello-world-piplinerun: invalid tekton bundle: 10.39.248.207:5000/tektonbundlesimproperformat:sha256:a4c4d878e0620aa3cfd29500afb032eb2acb8d8c787944673f3df8360541bdac does not contain a dev.tekton.image.name annotation)

    tektonbundles_test.go:274: Error waiting for PipelineRun to finish with expected error: "hello-world-piplinerun" completed with the wrong message: Pipeline arendelle-t6ll4/hello-world-pipeline-dne can't be Run; it contains Tasks that don't exist: Couldn't retrieve Task "hello-world-dne": invalid tekton bundle: registry:sha256:ddad3d7c1e96adf9153f8921a7c9790f880a390163df453be1566e9ef0d546e0 does not contain a dev.tekton.image.apiVersion annotation

Edit after some debugging: Ah ok, I think the failures are expected but we're supposed to receive "not found" errors instead of validation problems like we do here.

ghost · 2021-05-07T16:14:08Z

So it looks like the Resolver.Get() func enforces image compliance before checking that the expected resource exists inside the image. This is the source of the error message at least. What I don't totally understand atm is why the images under test are considered invalid. Both of the failing tests specify dev.tekton.image.name and dev.tekton.image.apiVersion annotations before publishing the image.

OK I get it now. Updated the test/tektonbundles_test.go file.

pierretasci · 2021-05-07T16:24:42Z

OK I get it now. Updated the test/tektonbundles_test.go file.

Thanks for fixing tha and sorry for only getting to this message now!

ghost · 2021-05-07T16:26:26Z

All good, thanks for taking a look!

ghost · 2021-05-07T16:28:09Z

@afrittoli I've added this to the 0.24 milestone as a late addition so that we're running tests against the "alpha" gate before the release goes live. Since this is so late in the window no problem at all if you want to keep it out of 0.24.0.

ghost · 2021-05-07T16:56:38Z

/test pull-tekton-pipeline-integration-tests

Prior to this commit the integration tests run against PRs were only those for "stable" features. This PR adds additional integration test runs for "alpha" features as well. `test/e2e-scripts.sh` now patches the feature-flags ConfigMap before executing integration tests and examples tests, first with "enable-api-fields: stable" and then with "enable-api-fields: alpha". As part of switching on alpha tests two issues have also been fixed with alpha features: 1. Sidecar workspaces could collide with existing volumeMounts attached to the sidecar if the mountpath of the workspace matched an existing volumeMount. This has been fixed by preventing the additional workspace volumeMount if one already exists at that mountpath. 2. Tekton bundles integration tests needed to be updated to account for validation errors that are returned when a bundle is invalid.

ghost · 2021-05-07T20:18:35Z

/test tekton-pipeline-unit-tests

ghost · 2021-05-07T20:36:36Z

Awesome, integration tests are passing with both "stable" and "alpha" flags set. 🎉

ghost · 2021-05-07T20:37:01Z

For the build tests, please see #3922

souleb · 2021-05-08T19:43:02Z

test/e2e-tests.sh

+set_feature_gate "stable"
+run_e2e
+
+set_feature_gate "alpha"
+run_e2e


vdemeester

So, we are running twice the e2e tests now right ? I think, long term, it should be two different job that can run in parallel but maybe the focus should be on using tekton pipelines for the CI before thinking about that 😛

Something I don't see in this PR though, is an example of a test that is gonna be skipped if the "alpha" feature gate is not set (aka if we are running on stable).

afrittoli · 2021-05-10T09:55:18Z

So, we are running twice the e2e tests now right ? I think, long term, it should be two different job that can run in parallel but maybe the focus should be on using tekton pipelines for the CI before thinking about that 😛

Indeed, I started working on running CI in kind so that we can run more jobs without using all our available projects. And using Tekton to define the test pipeline.

Something I don't see in this PR though, is an example of a test that is gonna be skipped if the "alpha" feature gate is not set (aka if we are running on stable).

We already have tests in the alpha folder - see in examples/v1beta1/pipelineruns/alpha

vdemeester · 2021-05-10T09:58:28Z

Something I don't see in this PR though, is an example of a test that is gonna be skipped if the "alpha" feature gate is not set (aka if we are running on stable).

We already have tests in the alpha folder - see in examples/v1beta1/pipelineruns/alpha

Ah ok, for the examples 👍🏼. But we might have to have some "helpers" on the go e2e tests 🙃

ghost · 2021-05-10T11:09:35Z

Something I don't see in this PR though, is an example of a test that is gonna be skipped if the "alpha" feature gate is not set (aka if we are running on stable).

@vdemeester this sounds like what you're after?

pipeline/test/tektonbundles_test.go

Lines 48 to 57 in 2266e18

    
           var requireFeatureFlags = requireAnyGate(map[string]string{ 
        
           	"enable-tekton-oci-bundles": "true", 
        
           	"enable-api-fields":         "alpha", 
        
           }) 
        
           // TestTektonBundlesSimpleWorkingExample is an integration test which tests a simple, working Tekton bundle using OCI 
        
           // images. 
        
           func TestTektonBundlesSimpleWorkingExample(t *testing.T) { 
        
           	ctx := context.Background() 
        
           	c, namespace := setup(ctx, t, withRegistry, requireFeatureFlags)

ghost · 2021-05-10T11:10:21Z

Specifically, requireAnyGate is a helper for e2e tests.

afrittoli · 2021-05-10T11:10:59Z

/test pull-tekton-pipeline-build-tests

afrittoli · 2021-05-10T12:06:52Z

test/e2e-tests.sh

+set_feature_gate "stable"
+run_e2e


NIT: it would be nice to be able to check that the controller has "seen" the configuration change before we start the tests. Not sure how, though?

afrittoli

Thank you!
/lgtm

afrittoli · 2021-05-10T12:11:09Z

Specifically, requireAnyGate is a helper for e2e tests.

Indeed, and we have a TestCustomTask and TestTektonBundles* using it.

vdemeester · 2021-05-10T12:31:15Z

/approve

tekton-robot · 2021-05-10T12:31:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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~~ [vdemeester]

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

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 6, 2021

tekton-robot requested review from bobcatfish and imjasonh May 6, 2021 17:38

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 6, 2021

tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label May 6, 2021

ghost changed the title ~~WIP Run our CI under "alpha" feature gate as well as "stable"~~ Add "alpha" feature gate tests (integration & examples) to our CI May 6, 2021

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 6, 2021

ghost commented May 6, 2021

View reviewed changes

ghost added this to the Pipelines v0.24 milestone May 7, 2021

souleb reviewed May 8, 2021

View reviewed changes

test/e2e-tests.sh

Comment on lines +55 to +59

set_feature_gate "stable"

run_e2e

set_feature_gate "alpha"

run_e2e

Copy link

Contributor

souleb May 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨

vdemeester reviewed May 10, 2021

View reviewed changes

afrittoli reviewed May 10, 2021

View reviewed changes

tekton-robot assigned afrittoli May 10, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2021

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2021

tekton-robot merged commit ea9158f into tektoncd:main May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "alpha" feature gate tests (integration & examples) to our CI #3917

Add "alpha" feature gate tests (integration & examples) to our CI #3917

ghost commented May 6, 2021 •

edited by ghost

Loading

ghost commented May 6, 2021

ghost May 6, 2021

tekton-robot commented May 6, 2021

tekton-robot commented May 6, 2021

tekton-robot commented May 6, 2021

ghost commented May 7, 2021 •

edited by ghost

Loading

ghost commented May 7, 2021 •

edited by ghost

Loading

pierretasci commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

souleb May 8, 2021

vdemeester left a comment

afrittoli commented May 10, 2021

vdemeester commented May 10, 2021

ghost commented May 10, 2021

ghost commented May 10, 2021

afrittoli commented May 10, 2021

afrittoli May 10, 2021

afrittoli left a comment

afrittoli commented May 10, 2021

vdemeester commented May 10, 2021

tekton-robot commented May 10, 2021

Add "alpha" feature gate tests (integration & examples) to our CI #3917

Add "alpha" feature gate tests (integration & examples) to our CI #3917

Conversation

ghost commented May 6, 2021 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

ghost commented May 6, 2021

ghost May 6, 2021

Choose a reason for hiding this comment

tekton-robot commented May 6, 2021

tekton-robot commented May 6, 2021

tekton-robot commented May 6, 2021

ghost commented May 7, 2021 • edited by ghost Loading

ghost commented May 7, 2021 • edited by ghost Loading

pierretasci commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

ghost commented May 7, 2021

souleb May 8, 2021

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

afrittoli commented May 10, 2021

vdemeester commented May 10, 2021

ghost commented May 10, 2021

ghost commented May 10, 2021

afrittoli commented May 10, 2021

afrittoli May 10, 2021

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli commented May 10, 2021

vdemeester commented May 10, 2021

tekton-robot commented May 10, 2021

ghost commented May 6, 2021 •

edited by ghost

Loading

ghost commented May 7, 2021 •

edited by ghost

Loading

ghost commented May 7, 2021 •

edited by ghost

Loading