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.

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

Add examples for v1beta1 apiVersion #1143

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented Jul 2, 2021

Changes

This commit does the following:

  1. Move existing examples to examples/v1alpha1
  2. Create a examples/v1beta1 for examples using v1beta1 apiVersion. Currently these are the same examples as v1alpha1 with the just the apiVersion set to v1beta1. The only exception is the webhook-interceptor which is not supported in v1beta1.
  3. Update test/e2e-tests-examples to run tests for both v1alpha1 and v1beta1.

Fixes #1067

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 2, 2021
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 2, 2021
@dibyom
Copy link
Member Author

dibyom commented Jul 2, 2021

/kind documentation

@tekton-robot tekton-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 2, 2021
@dibyom dibyom marked this pull request as ready for review July 2, 2021 20:29
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2021
@dibyom
Copy link
Member Author

dibyom commented Jul 2, 2021

/assign @khrm
/assign @savitaashture
/assign @wlynch

@bobcatfish
Copy link
Collaborator

just a drive by comment, might be worth importing the test runner we added to pipelines for running these: https://github.com/tektoncd/pipeline/blob/main/test/README.md#running-yaml-tests tektoncd/pipeline#2685

(just ignore me if that's already been done and im missing it)

@dibyom
Copy link
Member Author

dibyom commented Jul 12, 2021

just a drive by comment, might be worth importing the test runner we added to pipelines for running these: https://github.com/tektoncd/pipeline/blob/main/test/README.md#running-yaml-tests tektoncd/pipeline#2685

Took a quick look at the pipelines runner -- it seems like it is mostly unexported functions and the test runner is somewhat coupled to pipelines tests (e.g. specific mentions of clustertasks)...I think we can adopt some of the same approaches here but doesn't look like it is worth importing it in the current shape. We'd have add additional logic around port-forwarding to the EL, and sending over requests in either case. @sm43 has been tackling some of this as part of #192

Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Few minor formatting things I'd like to go back and fix, but nothing worth blocking this PR for.

It does feel weird that we're copying so much - I wonder if there's a less duplicative way to represent these. 🤔

examples/v1alpha1/eventlistener-tls-connection/README.md Outdated Show resolved Hide resolved
examples/v1alpha1/bitbucket/README.md Outdated Show resolved Hide resolved
@dibyom dibyom force-pushed the v1beta1-examples branch 2 times, most recently from bc1b86e to 7557db6 Compare July 15, 2021 16:36
@dibyom
Copy link
Member Author

dibyom commented Jul 15, 2021

It does feel weird that we're copying so much - I wonder if there's a less duplicative way to represent these. 🤔

Yeah I tried playing around with a different setup - a single triggers folder that contains all triggers... but that became a bit unwieldy since each trigger required its own HTTP request body that was different from others.

@dibyom dibyom force-pushed the v1beta1-examples branch from 7557db6 to 75dda1f Compare July 15, 2021 20:45
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2021
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wlynch

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 Jul 15, 2021
@dibyom dibyom force-pushed the v1beta1-examples branch from 75dda1f to 83c8807 Compare July 15, 2021 21:20
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2021
This commit does the following:
1. Move existing examples to examples/v1alpha1
2. Create a examples/v1beta1 for examples using v1beta1 apiVersion. Currently these are the same examples as v1alpha1 with the just the apiVersion set to v1beta1. The only exception is the webhook-interceptor which is not supported in v1beta1.
3. Update test/e2e-tests-examples to run tests for both v1alpha1 and v1beta1.

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom dibyom force-pushed the v1beta1-examples branch from 83c8807 to 92dbf3d Compare July 15, 2021 21:39
Copy link
Contributor

@savitaashture savitaashture left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2021
@tekton-robot tekton-robot merged commit f2380d7 into tektoncd:main Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/documentation Categorizes issue or PR as related to documentation. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V1Beta1 types for Triggers
6 participants