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

Rename containerTemplate to stepTemplate 👠 #931

Merged
merged 2 commits into from
Jun 28, 2019

Conversation

bobcatfish
Copy link
Collaborator

@bobcatfish bobcatfish commented May 29, 2019

Changes

We used the term containerTemplate for this template b/c it is a
template of the containers used for each step - however we don't refer
to those as containers, we refer to them as steps, so I think calling
this stepTemplate makes slightly more sense and makes the relationship
between this template + the steps a bit more obvious.

This change is backward compatible b/c both containerTemplate and
stepTemplate will be supported until #977 where we will make the
backward incompatible change and remove containerTemplate.

Submitter Checklist

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

See the contribution guide
for more details.

Release Notes

🚨 Please migrate from `containerTemplate` to `stepTemplate` 🚨
The field `containerTemplate` in `Task` is now called `stepTemplate`. `containerTemplate` will be removed in the next release (#977)

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label May 29, 2019
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 29, 2019
@bobcatfish
Copy link
Collaborator Author

@abayer I think this is relevant to your interests! 🙏

@bobcatfish
Copy link
Collaborator Author

And this is an API change so would like to review from several of @vdemeester @dlorenc @imjasonh ❤️

@vdemeester
Copy link
Member

/hold
(so that we have time to review 👼)

@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 May 30, 2019
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.

I am 👍 on this change as it's more coherent with the steps field.
We will need a huge warning in the release note. I don't think there is any task in the catalog using containerTemplate yet, but we will need to make sure and follow-up in there. Also, the fact that this is green, it feels we don't have yaml tests using containerTemplate here, would be cool to have some 👼

@abayer
Copy link
Contributor

abayer commented May 30, 2019

/lgtm

I’m a strong +1 on this. It’s definitely a more explanatory name!

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2019
docs/tasks.md Outdated Show resolved Hide resolved
@bobcatfish
Copy link
Collaborator Author

We will need a huge warning in the release note.

maybe we can have a section in the future for "backwards incompatible changes". Leave this one with me! :D

Also, the fact that this is green, it feels we don't have yaml tests using containerTemplate here, would be cool to have some angel

Added!

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2019
@bobcatfish
Copy link
Collaborator Author

PTAL :D

/meow space

@tekton-robot
Copy link
Collaborator

@bobcatfish: cat image

In response to this:

PTAL :D

/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@abayer
Copy link
Contributor

abayer commented May 31, 2019

/lgtm
/meow box

@tekton-robot
Copy link
Collaborator

@abayer: Bad category. Please see https://api.thecatapi.com/api/categories/list

In response to this:

/lgtm
/meow box

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2019
@abayer
Copy link
Contributor

abayer commented May 31, 2019

/meow boxes

Dang it. =)

@tekton-robot
Copy link
Collaborator

@abayer: Bad category. Please see https://api.thecatapi.com/api/categories/list

In response to this:

/meow boxes

Dang it. =)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@abayer
Copy link
Contributor

abayer commented May 31, 2019

/meow boxes

@tekton-robot
Copy link
Collaborator

@abayer: cat image

In response to this:

/meow boxes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bobcatfish
Copy link
Collaborator Author

it was all worth it 🐱

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.

/lgtm

@vdemeester
Copy link
Member

cc @dlorenc @imjasonh for last reviews 👼

@bobcatfish
Copy link
Collaborator Author

@imjasonh suggested that we use this change to try out rolling out a backward incompatible change across 2 releases so I'm gonna do that!

@afrittoli
Copy link
Member

The most direct consumers of the pipeline API are the other Tekton projects, so we could try and make sure we coordinate the release with them.
How's the roll out across 2 releases going to work?

@bobcatfish
Copy link
Collaborator Author

How's the roll out across 2 releases going to work?

  1. A release would go out that supports both the old way and the new way, with a warning
  2. The next release would actually drop the old way and be backward incompatible

There's more about this at https:/tektoncd/pipeline/blob/master/api_compatibility_policy.md#possibly-beta-in-03 (where we theorize we might go beta in 0.3 haha... maybe we should go beta now though, it seems like folks are surprised by the idea that we might make a backward incompatible change? 🤔 )

@vdemeester
Copy link
Member

How's the roll out across 2 releases going to work?

1. A release would go out that supports both the old way and the new way, with a warning
2. The next release would actually drop the old way and be backward incompatible

There's more about this at https:/tektoncd/pipeline/blob/master/api_compatibility_policy.md#possibly-beta-in-03 (where we theorize we might go beta in 0.3 haha... maybe we should go beta now though, it seems like folks are surprised by the idea that we might make a backward incompatible change? thinking )

So, overall we could have something like that. We should try to follow Kubernetes deprecation policy as much as possible.

Rule #1: API elements may only be removed by incrementing the version of the API group.
Rule #2: API objects must be able to round-trip between API versions in a given release without information loss, with the exception of whole REST resources that do not exist in some versions.
Rule #3: An API version in a given track may not be deprecated until a new API version at least as stable is released.
Rule #4a: Other than the most recent API versions in each track, older API versions must be supported after their announced deprecation for a duration of no less than:
GA: 12 months or 3 releases (whichever is longer)
Beta: 9 months or 3 releases (whichever is longer)
Alpha: 0 releases
Rule #4b: The “preferred” API version and the “storage version” for a given group may not advance until after a release has been made that supports both the new version and the previous version

As we are in Alpha right now, we don't really have anything to do, this is the reason why we use v1alpha1 on Tekton API right now, is so that people building on it know that we might make a backward incompatible change. That's why I think only a release note can be sufficient for that, and for now.

We could also, bump the version to v1alpha2, which could help us do 2 things:

  • for people building on top of tekton to migrate at their pace on a new API
  • for us to manage multiple api version as it's gonna be required in the near future anyway 👼

And, this brings to the table a discussion about how and when to promote our API to the Beta phase, aka v1beta1, v1beta2, …

@bobcatfish
Copy link
Collaborator Author

Created #977 to actually make the backward incompatible change.

@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2019
@bobcatfish
Copy link
Collaborator Author

Okay I've updated the change in this PR to be backward compatible for now - PTAL @vdemeester @abayer @chmouel @imjasonh @afrittoli 😸

@bobcatfish bobcatfish force-pushed the stepTemplate branch 2 times, most recently from 668fe1c to 53db5c9 Compare June 11, 2019 23:22
@vdemeester
Copy link
Member

@bobcatfish needs a rebase 😓 otherwise looking good (took the same approach for #1004 )

We used the term `containerTemplate` for this template b/c it is a
template of the containers used for each step - however we don't refer
to those as containers, we refer to them as steps, so I think calling
this `stepTemplate` makes slightly more sense and makes the relationship
between this template + the steps a bit more obvious.

This change is backward compatible b/c both `containerTemplate` and
`stepTemplate` will be supported until tektoncd#977 where we will make the
backward incompatible change and remove `containerTemplate`.
There weren't any "yaml tests" aka examples that used the stepTemplate
so after I renamed `stepTemplate` from `containerTemplate` it was not
100% clear if this worked. Also it's helpful to provide examples to
users to see it in action :)
@bobcatfish
Copy link
Collaborator Author

This should be rebased and ready to go now @vdemeester !

/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 Jun 27, 2019
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.

/lgtm
A quick update is gonna be needed in the tektoncd/catalog once we do a release

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, chmouel, 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 [bobcatfish,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 tekton-robot merged commit 60ce931 into tektoncd:master Jun 28, 2019
bobcatfish added a commit to bobcatfish/pipeline that referenced this pull request Aug 7, 2019
In tektoncd#931 we added `stepTemplate` in addition to `containerTemplate` and
this chagne went out with 0.5. In this release we can now remove
containerTemplate completely.

Fixes tektoncd#977
tekton-robot pushed a commit that referenced this pull request Aug 8, 2019
In #931 we added `stepTemplate` in addition to `containerTemplate` and
this chagne went out with 0.5. In this release we can now remove
containerTemplate completely.

Fixes #977
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants