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

Bump tekton to v0.21.0 release #663

Merged

Conversation

xiujuan95
Copy link
Contributor

@xiujuan95 xiujuan95 commented Mar 15, 2021

Changes

Bump tekton to v0.21.0 release.

  • Changes for go modules and vendor Dependencies
  • Modify docs and tekton installation script with new version

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

  • Tekton dependency change
Tekton version is bumped to v0.21.0

@openshift-ci-robot openshift-ci-robot added the release-note Label for when a PR has specified a release note label Mar 15, 2021
@xiujuan95 xiujuan95 changed the title Bump tekton to v0.21.0 release [WIP] Bump tekton to v0.21.0 release Mar 15, 2021
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2021
@xiujuan95 xiujuan95 force-pushed the bump-tekton-to-v0.21.0 branch from 7f1f4d4 to 09c87a5 Compare March 15, 2021 10:14
@qu1queee
Copy link
Contributor

/retest

@qu1queee qu1queee self-requested a review March 15, 2021 10:39
@gabemontero
Copy link
Member

The description needs to be updated with the actual release note text. Please add at the bottom

Tekton dependency bumped to v0.21.0

/hold

feel free to cancel the hold once the description is updated.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2021
@gabemontero
Copy link
Member

See https://raw.githubusercontent.com/shipwright-io/build/master/.github/pull_request_template.md if you want to verify the precise syntax for the release note .. i.e. 4 back ticks followed by release-note

@@ -26,7 +26,7 @@ as [Kaniko](https://github.com/GoogleContainerTools/kaniko),
| Dependency | Supported versions |
| ----------------------------------------- | -------------------------------------- |
| [Kubernetes](https://kubernetes.io/) | v1.15.\*, v1.16.\*, v1.17.\*, v1.18.\* |
| [Tekton](https://cloud.google.com/tekton) | v0.20.1 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we describe the minimum version of Tekton that's required by Shipwright? This makes it seem like 0.21 is the minimum, which might make it harder for people to try out.

Same with minimum Kubernetes versions above; saying 1.15 - 1.18 makes it sound like 1.19+ aren't supported for some reason.

Copy link
Contributor Author

@xiujuan95 xiujuan95 Mar 16, 2021

Choose a reason for hiding this comment

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

Thanks @imjasonh !
Currently, in our side, we don't have a flow to test the minimum version of tekton for each shipwright-io build release. At present, for each build release, we only bump tekton to the version what we want and test it.

So, to some extent, for a fixed build release, users need to have a fixed tekton version. But yes, indeed, this is a good suggestion. Maybe we can add a flow to test the minimum tekton version for each build release in the future, then we can document it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand we might not have tests for every Tekton version, but I think if we believe it works with 0.20 or 0.19 we should document that. If lacking tests is a concern we can invest in that as well. GitHub Actions can run against a matrix of Tekton versions relatively easily if we feel that's necessary to test.

Shipwright should be expected to work with a range of Tekton versions, or else there's no way to upgrade either Tekton or Shipwright safely, since they'd have to be done in lock-step at the same time. Users who have an older Tekton version (even just the previous version) might be scared off to learn they need the absolute latest Tekton release to use Shipwright.

This feedback doesn't need to block this PR, but we should definitely not be documenting that Shipwright only works with one version of Tekton.

Copy link
Contributor

Choose a reason for hiding this comment

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

@imjasonh we tried the matrix before for the k8s version permutations. I think github actions started to behave strange. I need to revisit what was exactly the problem. All I´m trying to say is that infrastructure wise, github might not be ideal for so many flavors of testing, we need to revisit this again.

I see the value on not scaring users in regards of Tekton versions. So we definitely need to do better in here.

I will open a new issue in our backlog that covers:

  • How to properly document minimum version for k8s and tekton, for Shipwright users in order to avoid situations as the one you jsut mentioned on might be scared.
  • Provide a follow-up on how can we have a test that can continuously test N,N-1,N-2 versions of Tekton and k8s. Plus a layout of pros and cons based on what we found when @SaschaSchwarze0 tried it.

README.md Outdated Show resolved Hide resolved
@xiujuan95 xiujuan95 force-pushed the bump-tekton-to-v0.21.0 branch 6 times, most recently from 9a08ee5 to 5158324 Compare March 16, 2021 04:00
@openshift-ci-robot openshift-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Label for when a PR has specified a release note labels Mar 16, 2021
@xiujuan95 xiujuan95 force-pushed the bump-tekton-to-v0.21.0 branch 8 times, most recently from d12edee to 98b9f8e Compare March 16, 2021 09:24
@xiujuan95 xiujuan95 changed the title [WIP] Bump tekton to v0.21.0 release Bump tekton to v0.21.0 release Mar 16, 2021
@xiujuan95 xiujuan95 requested a review from imjasonh March 16, 2021 09:50
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2021
@xiujuan95
Copy link
Contributor Author

/unhold

@openshift-ci-robot openshift-ci-robot added release-note Label for when a PR has specified a release note and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 16, 2021
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

@xiujuan95 thanks, nice work. This was actually blocked before, but is nice to see that we finally are able to bump our two core dependencies. I´m asking for minor changes, in general the PR looks good.

go.mod Outdated
k8s.io/kube-openapi => k8s.io/kube-openapi v0.0.0-20200410145947-bcb3869e6f29 // resolve `case-insensitive import collision` for gnostic/openapiv2 package
sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.6.1
)
replace github.com/Azure/go-autorest => github.com/Azure/go-autorest v14.2.0+incompatible // Required by OLM
Copy link
Contributor

@qu1queee qu1queee Mar 16, 2021

Choose a reason for hiding this comment

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

running go mod tidy seems to always want to add a new line, so maybe you should have that one here. Otherwise anyone else running a go mod tidy in the future might experience a change in the go.mod. Does this make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense, done!

go.mod Show resolved Hide resolved
run make targets to generate new things
@xiujuan95 xiujuan95 force-pushed the bump-tekton-to-v0.21.0 branch from 0da8e20 to f9aa2b4 Compare March 17, 2021 03:52
@xiujuan95 xiujuan95 force-pushed the bump-tekton-to-v0.21.0 branch from f9aa2b4 to fe16158 Compare March 17, 2021 03:59
@xiujuan95 xiujuan95 requested a review from qu1queee March 17, 2021 04:03
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qu1queee

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit c3c3e88 into shipwright-io:master Mar 17, 2021
@qu1queee qu1queee added this to the release-v0.4.0 milestone Mar 24, 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. lgtm Indicates that a PR is ready to be merged. release-note Label for when a PR has specified a release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants