remove PipelineStatus and PipelineResourceStatus #1640

poy · 2019-11-27T23:26:53Z

Changes

This removes two struct types:

PipelineStatus
PipelineResourceStatus

These Status types were unused and did not follow the pattern of using
Knative's DuckType. Other types such as Task do not have a Status type
and instead defer to their corresponding TaskRun status.

fixes #1636

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

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Removes unused `status` fields from `Pipeline` and `PipelineResource`.

tekton-robot · 2019-11-27T23:27:08Z

Hi @poy. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vdemeester · 2019-11-28T09:20:52Z

/ok-to-test

pkg/pod/entrypoint.go

poy · 2019-11-30T22:11:23Z

~~Are we not using go 1.13?~~ NVM. Thought the test failed because of something else.

poy · 2019-12-01T05:06:52Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2019-12-02T09:10:17Z

/hold
/lgtm
Let's wait for 0.9.0 to be released before pushing this change.

vdemeester · 2019-12-03T10:09:09Z

@poy can you squash your commits ? 👼

bobcatfish · 2019-12-03T16:13:04Z

I updated the release notes in the PR desciription

vdemeester · 2019-12-05T16:10:48Z

@poy also needs a rebase 🙇‍♂️

This removes two struct types: * `PipelineStatus` * `PipelineResourceStatus` These Status types were unused and did not follow the pattern of using Knative's DuckType. Other types such as `Task` do not have a Status type and instead defer to their corresponding `TaskRun` status. fixes tektoncd#1636

poy · 2019-12-05T22:54:15Z

@vdemeester Rebased and squashed!

vdemeester

/hold cancel
/lgtm
/meow

Thanks @poy 🤗

tekton-robot · 2019-12-06T11:37:03Z

@vdemeester:

In response to this:

/hold cancel
/lgtm
/meow

Thanks @poy 🤗

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 · 2019-12-06T11:37:05Z

[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

bobcatfish · 2019-12-17T19:35:39Z

@vdemeester sounds like you're already on this but i think this has some implications for CLI + Dashboard + and also Triggers (@dibyom ) - I just tried to use latest triggers with latest pipelines and got:

"Could not create resources for \"\": couldn't create resource with group version kind \"tekton.dev/v1alpha1, Resource=pipelineresources\": Internal error occurred: admission webhook \"webhook.tekton.dev\" denied the request: mutation failed: cannot decode incoming new object: json: unknown field \"status\""

vdemeester · 2019-12-18T10:58:18Z

@bobcatfish ah interesting… because there was no omitempty.. yeah… we need to bump the dependency to have the updated struct to have it work… that's not the best then because this will break user…

They are not used, and shouldn't be there, but client that have previous version of this struct would fail targeting the API. This is a *partial* revert of tektoncd#1640, as it doesn't reintroduce the `UpdateStatus` generation. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester · 2019-12-18T11:12:09Z

See #1762 for backward compat.

They are not used, and shouldn't be there, but client that have previous version of this struct would fail targeting the API. This is a *partial* revert of tektoncd#1640, as it doesn't reintroduce the `UpdateStatus` generation. Signed-off-by: Vincent Demeester <[email protected]>

They are not used, and shouldn't be there, but client that have previous version of this struct would fail targeting the API. This is a *partial* revert of #1640, as it doesn't reintroduce the `UpdateStatus` generation. Signed-off-by: Vincent Demeester <[email protected]>

They are not used, and shouldn't be there, but client that have previous version of this struct would fail targeting the API. This is a *partial* revert of tektoncd#1640, as it doesn't reintroduce the `UpdateStatus` generation. Signed-off-by: Vincent Demeester <[email protected]>

They are not used, and shouldn't be there, but client that have previous version of this struct would fail targeting the API. This is a *partial* revert of #1640, as it doesn't reintroduce the `UpdateStatus` generation. Signed-off-by: Vincent Demeester <[email protected]>

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 27, 2019

tekton-robot requested review from dibyom and vdemeester November 27, 2019 23:26

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 27, 2019

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 28, 2019

vdemeester approved these changes Nov 28, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2019

dlorenc reviewed Nov 29, 2019

View reviewed changes

pkg/pod/entrypoint.go Outdated Show resolved Hide resolved

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, 2019

tekton-robot assigned vdemeester Dec 2, 2019

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 2, 2019

poy force-pushed the fix/1636 branch from 2108040 to 4081d61 Compare December 5, 2019 22:53

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2019

vdemeester approved these changes Dec 6, 2019

View reviewed changes

tekton-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 6, 2019

tekton-robot merged commit e75dbaa into tektoncd:master Dec 6, 2019

vdemeester mentioned this pull request Dec 18, 2019

Keep Pipeline.status field for backward compatibility 👾 #1762

Merged

3 tasks

vdemeester mentioned this pull request Dec 19, 2019

Keep PipelineResource.status field for backward compatibility 👾 #1766

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove PipelineStatus and PipelineResourceStatus #1640

remove PipelineStatus and PipelineResourceStatus #1640

poy commented Nov 27, 2019 •

edited by bobcatfish

Loading

tekton-robot commented Nov 27, 2019

vdemeester commented Nov 28, 2019

poy commented Nov 30, 2019 •

edited

Loading

poy commented Dec 1, 2019

vdemeester commented Dec 2, 2019

vdemeester commented Dec 3, 2019

bobcatfish commented Dec 3, 2019

vdemeester commented Dec 5, 2019

poy commented Dec 5, 2019

vdemeester left a comment

tekton-robot commented Dec 6, 2019

tekton-robot commented Dec 6, 2019

bobcatfish commented Dec 17, 2019

vdemeester commented Dec 18, 2019

vdemeester commented Dec 18, 2019

remove PipelineStatus and PipelineResourceStatus #1640

remove PipelineStatus and PipelineResourceStatus #1640

Conversation

poy commented Nov 27, 2019 • edited by bobcatfish Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Nov 27, 2019

vdemeester commented Nov 28, 2019

poy commented Nov 30, 2019 • edited Loading

poy commented Dec 1, 2019

vdemeester commented Dec 2, 2019

vdemeester commented Dec 3, 2019

bobcatfish commented Dec 3, 2019

vdemeester commented Dec 5, 2019

poy commented Dec 5, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 6, 2019

tekton-robot commented Dec 6, 2019

bobcatfish commented Dec 17, 2019

vdemeester commented Dec 18, 2019

vdemeester commented Dec 18, 2019

poy commented Nov 27, 2019 •

edited by bobcatfish

Loading

poy commented Nov 30, 2019 •

edited

Loading