Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add some switches to the e2e script ⚙️ #4400

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Add some switches to the e2e script ⚙️ #4400

merged 1 commit into from
Mar 28, 2022

Conversation

afrittoli
Copy link
Member

@afrittoli afrittoli commented Nov 29, 2021 •
edited
Loading

Changes

Add three switches (env variables) in the main e2e script, that
can be used to:

  • skip setting up the test environment
  • skip running the YAML based tests
  • set the feature gate (this is also available as ENV var

They are useful for setting up the E2E tests in KinD clusters, as
well as generally useful for development purposes.

Add an env file which is used by the kind e2e script, see
tektoncd/plumbing#927 for reference

Signed-off-by: Andrea Frittoli andrea.frittoli@uk.ibm.com

/kind misc

Submitter Checklist

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

  • Docs included if any changes are user facing
  • 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

NONE

@tekton-robot tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/misc Categorizes issue or PR as a miscellaneuous one. labels Nov 29, 2021
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 29, 2021
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 29, 2021
@afrittoli
Copy link
Member Author

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2021
@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 30, 2021
@afrittoli
Copy link
Member Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2021
vdemeester
vdemeester approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

We discussed this a little bit for the operator as well but, shouldn't we, most likely invert those flags ?
My rational is : as a contributor, what is the main use case I will have when running e2e tests ? Most likely, exercising what I just wrote and deploy on my cluster right ? So most of the time I will want to skip the installation (and cluster provisionning, …). That part is mainly useful for CI, thus I tend to think it is better to make the CI have some environment variable than making the contributor write those each time he wants to try and run e2e tests on his work-in-progress.

@tekton-robot
Copy link
Collaborator

[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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2021
@afrittoli
Copy link
Member Author

We discussed this a little bit for the operator as well but, shouldn't we, most likely invert those flags ? My rational is : as a contributor, what is the main use case I will have when running e2e tests ? Most likely, exercising what I just wrote and deploy on my cluster right ? So most of the time I will want to skip the installation (and cluster provisionning, …). That part is mainly useful for CI, thus I tend to think it is better to make the CI have some environment variable than making the contributor write those each time he wants to try and run e2e tests on his work-in-progress.

Yeah, I think in development use cases one would like to deploy Tekton (including the code changes) and run tests.
Running initialize won't be needed then and not doing that could be the default, but that will require then updating all existing e2e prow jobs.

@vdemeester
Copy link
Member

Yeah, I think in development use cases one would like to deploy Tekton (including the code changes) and run tests. Running initialize won't be needed then and not doing that could be the default, but that will require then updating all existing e2e prow jobs.

Yes agreed, hence me approving. I think we can do this as follow-ups but not need to it now, at once.

@afrittoli
Copy link
Member Author

@imjasonh @bobcatfish can I get a second review on this - I'd love to move forward with kind testing for pipeline asap - so hopefully we can get it up and running before the holidays

@nikhil-thomas
Copy link
Member

👍 lgtm

@nikhil-thomas nikhil-thomas mentioned this pull request Dec 6, 2021
Closed
4 tasks
@imjasonh
Copy link
Member

imjasonh commented Dec 6, 2021

Looks good to me. If @bobcatfish doesn't have comments I can give it the official slash-lgtm.

@afrittoli
Copy link
Member Author

/test kind

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

Added tektoncd/plumbing#942 to plumbing to that the job does not skip

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

1 similar comment
@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test check-pr-has-kind-label

@ghost
Copy link

ghost commented Jan 11, 2022

/lgtm

@tekton-robot tekton-robot assigned ghost Jan 11, 2022
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2022
@bobcatfish
Copy link
Collaborator

not blocking the PR but a request and a thought:

  • making our already complicated bash based testing logic more complicated feels a bit 😞 but we've been talking about migrating it to tekton for this long and still havent so i dont want to get in the way of adding functionality we need
  • could there be some comments in the script that explain what each of the flags are for?

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test check-pr-has-kind-label

@afrittoli
Copy link
Member Author

/test pull-tekton-pipeline-alpha-integration-tests

@afrittoli
Copy link
Member Author

/test check-pr-has-kind-label

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

3 similar comments
@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@pritidesai
Copy link
Member

hey @afrittoli is this ready to merge? Looks like this is stuck with some of the checks pull-pipeline-kind-k8s-v1-20-e2e and check-pr-has-kind-label .

Also, do you want to address @bobcatfish's comments in this PR?

@afrittoli
Copy link
Member Author

/test check-pr-has-kind-label

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

2 similar comments
@vdemeester
Copy link
Member

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

/test check-pr-has-kind-label

jerop
jerop reviewed Mar 15, 2022
View reviewed changes
Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

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

/test pull-pipeline-kind-k8s-v1-20-e2e

@jerop
Copy link
Member

jerop commented Mar 15, 2022

/retest

@jerop jerop closed this Mar 15, 2022
@jerop jerop reopened this Mar 15, 2022
@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-20-e2e

1 similar comment
@jerop
Copy link
Member

jerop commented Mar 25, 2022

/test pull-pipeline-kind-k8s-v1-20-e2e

@afrittoli
Copy link
Member Author

k8s v1.20 is not supported by Tekton anymore, so updating the CI job to use v1.21 tektoncd/plumbing#1034

@afrittoli
Copy link
Member Author

/pull-pipeline-kind-k8s-v1-21-e2e

@afrittoli
Copy link
Member Author

/test pull-pipeline-kind-k8s-v1-21-e2e

@tekton-robot tekton-robot merged commit 596b50b into tektoncd:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerop jerop jerop left review comments

@vdemeester vdemeester vdemeester approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@afrittoli @tekton-robot @vdemeester @nikhil-thomas @imjasonh @bobcatfish @pritidesai @jerop